Refactor Link to add more cheap unit tests #4553

OleksandrNechai · 2024-09-12T15:18:28Z

[FX-NNNN]

Description

This if a follow up of f8e5c8f and #4547. Where I did not test code due to inability to add cheap test.

Extract all view logic into separate pure function. This allows adding many tests that run in few ms, raising confidence in our code without making tests suits running forever.

How to test

Locally yarn test:unit:watch

Development checks

Breaking change

codemod is created and showcased in the changeset
test alpha package of Picasso in StaffPortal

All development checks should be done and set checked to pass the
GitHub Bot: TODOLess action

PR commands

List of available commands:

@toptal-bot run package:alpha-release - Release alpha version
@toptal-anvil ping reviewers - Ping FX team for review

PR Review Guidelines

When to approve? ✅

You are OK with merging this PR and

You have no extra requests.
You have optional requests.
1. Add nit: to your comment. (ex. nit: I'd rename this variable from makeCircle to getCircle)

When to request changes? ❌

You are not OK with merging this PR because

Something is broken after the changes.
Acceptance criteria is not reached.
Code is dirty.

When to comment (neither ✅ nor ❌)

You want your comments to be addressed before merging this PR in cases like:

There are leftovers like unnecessary logs, comments, etc.
You have an opinionated comment regarding the code that requires a discussion.
You have questions.

How to handle the comments?

An owner of a comment is the only one who can resolve it.
An owner of a comment must resolve it when it's addressed.
A PR owner must reply with ✅ when a comment is addressed.

changeset-bot · 2024-09-12T15:18:32Z

⚠️ No Changeset found

Latest commit: b32a83c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

toptal-devbot · 2024-09-12T15:19:44Z

	Fails
🚫	The pull request title is missing a Jira issue code. Correct format '[ASD-123] Add a cool feature'. If you're working without a Jira issue then add a 'no-jira' label to your pull request.

Generated by 🚫 dangerJS against b32a83c

OleksandrNechai · 2024-09-12T15:23:14Z

packages/base/Link/src/Link/Link.tsx

+  nativeHTMLAttributes: Omit<Props, keyof BaseProps>
+}
+
+/* eslint-disable complexity */


ES lint wants code to have no more 10 branches. For this kind of function this does not make sense, code is not complex (this error existed before refactoring too)

OleksandrNechai · 2024-09-12T15:24:01Z

packages/base/Link/src/Link/Link.tsx

+}
+
+/* eslint-disable complexity */
+export const calculateViewModel = (props: Props): ViewModel => {


This pure function return all info needed to render Link without rendering Link.

@OleksandrNechai Do you think that calculateViewModel might do too many things at once? I tried to split the returned pieces of "model" into groups, but have not succeeded, though.

In terms of code structure the function seems to be a good approach, but I am a bit worried that it does so many unrelated calculations, as in case of Link component the model itself has so many heterogeneous properties, so it might affect the readability a bit.

Yes, it totally does a lot of things. And yet I find this function simple because when you look at it, each section of code calculates one prop and they are mostly not intertwined. I tried to decompose this function in smaller ones, and result was not great. Extra functions would take 5 params and return one output and hide 2 lines of code. This make code more complicated, as I did not manage to extract "deep" function: https://medium.com/@nathan.fooo/4-notes-modules-should-be-deep-ba5671c4288c

@OleksandrNechai Agree!
Although the function still looks shallow as on the picture from https://medium.com/@nathan.fooo/4-notes-modules-should-be-deep-ba5671c4288c, I have not strong preference 👍

sashuk · 2024-09-12T15:58:13Z

packages/base/Link/src/Link/test.tsx

@@ -99,3 +100,222 @@ describe('Link', () => {
    expect(getByTestId('foo')).not.toHaveAttribute('href')
  })
 })
+
+describe('calculateViewModel', () => {
+  it('should apply default values when no props are provided', () => {


NIT: from https://toptal-core.atlassian.net/wiki/spaces/ENG/pages/1120830304/Frontend+Testing+-+Unit+testing#It

Do not start test description with should, because it doesn’t bring value.

sashuk · 2024-09-12T15:59:10Z

packages/base/Link/src/Link/test.tsx

+    expect(result.ariaDisabled).toBe(true)
+  })
+
+  it('should apply visited class when visited is true', () => {


NIT: from https://toptal-core.atlassian.net/wiki/spaces/ENG/pages/1120830304/Frontend+Testing+-+Unit+testing#Describe

We have to wrap the use case with describe section to accentuate a condition when that test case must happen.

sashuk · 2024-09-13T17:45:14Z

packages/base/Link/src/Link/test.tsx

+  })
+
+  describe('when href, target, rel and onClick are provided', () => {
+    it('applies correct href, target, rel, and onClick', () => {


@OleksandrNechai I wonder what does it mean "correct" href, target, etc.? Describing it might take more time than writing tests, but then maybe there is no value in this test?

ruslan-sed

Great job!
Very nice way to simplify component testing by extracting logic to helper function 👍

ruslan-sed · 2024-09-16T13:57:26Z

packages/base/Link/src/Link/test.tsx

+      const result = calculateViewModel(props)
+
+      expect(result.href).toBe('https://example.com')
+      expect(result.target).toBe('_blank')


Maybe let's use a variable for those values?

sashuk

Looks good!

OleksandrNechai requested a review from a team as a code owner September 12, 2024 15:18

OleksandrNechai self-assigned this Sep 12, 2024

OleksandrNechai commented Sep 12, 2024

View reviewed changes

sashuk reviewed Sep 12, 2024

View reviewed changes

sashuk requested a review from a team September 12, 2024 15:59

sashuk reviewed Sep 13, 2024

View reviewed changes

ruslan-sed self-requested a review September 16, 2024 10:10

ruslan-sed approved these changes Sep 16, 2024

View reviewed changes

sashuk approved these changes Sep 17, 2024

View reviewed changes

OleksandrNechai force-pushed the fx-refactor-link-away-from-being-too-complex branch from 4d500ab to a224ead Compare September 17, 2024 20:48

OleksandrNechai added 7 commits September 18, 2024 10:53

Refactor Link to add more cheap unit tests

3efab02

Add few more props

b1a1f37

Remove redundant comments

bc0e1fe

Remove should from tests

8678a1a

Refactor tests to meet guidelines

07e4a31

Update snapshots

85e4452

Clarify 'when href, target, rel and onClick' test

b32a83c

OleksandrNechai force-pushed the fx-refactor-link-away-from-being-too-complex branch from a224ead to b32a83c Compare September 18, 2024 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Link to add more cheap unit tests #4553

Refactor Link to add more cheap unit tests #4553

OleksandrNechai commented Sep 12, 2024 •

edited

Loading

changeset-bot bot commented Sep 12, 2024 •

edited

Loading

toptal-devbot commented Sep 12, 2024 •

edited

Loading

OleksandrNechai Sep 12, 2024

OleksandrNechai Sep 12, 2024

sashuk Sep 13, 2024

OleksandrNechai Sep 13, 2024

sashuk Sep 13, 2024

sashuk Sep 12, 2024

sashuk Sep 12, 2024

sashuk Sep 13, 2024

ruslan-sed left a comment

ruslan-sed Sep 16, 2024

sashuk left a comment

Refactor Link to add more cheap unit tests #4553

Are you sure you want to change the base?

Refactor Link to add more cheap unit tests #4553

Conversation

OleksandrNechai commented Sep 12, 2024 • edited Loading

Description

How to test

Development checks

When to approve? ✅

When to request changes? ❌

When to comment (neither ✅ nor ❌)

How to handle the comments?

changeset-bot bot commented Sep 12, 2024 • edited Loading

⚠️ No Changeset found

toptal-devbot commented Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruslan-sed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sashuk left a comment

Choose a reason for hiding this comment

OleksandrNechai commented Sep 12, 2024 •

edited

Loading

changeset-bot bot commented Sep 12, 2024 •

edited

Loading

toptal-devbot commented Sep 12, 2024 •

edited

Loading