refactor: [M3-8291] - Enforce Factory.each to start increment at `1` #10619

abailly-akamai · 2024-06-26T21:49:18Z

Description 📝

This PR makes sure Factory.each always start iterating from 1 rather than 0. Having falsy entity IDs is responsible for for a few issues (unit tests false positives in, disabling queries based on a truthy entity id) and will help us with our "Next Gen MSW ©" where we will generate new entities based on factories and those can't have 0 as an id.

In order to do so I added a monkey patch (run time modifier) for Factory.each so that it increments from 1 in every factory without having to modify every factory instance. This means not only IDs but also every value relying on an each was modified. As a result, many tests asserting either for some order, value or array length needed to be updated. It also surfaced a few issues with some tests, tho more could have been fixed (outside of scope for this PR).

This PR does not touch any consumer code.

Changes 🔄

Patch Factory.js each to increment from 1
Fix tests affected

How to test 🧪

Verification steps

run yarn test and confirm all tests are passing locally and in CI
confirm e2e suites are passing in CI

As an Author I have considered 🤔

Check all that apply

abailly-akamai · 2024-06-27T02:02:36Z

...features/Databases/DatabaseDetail/DatabaseResize/DatabaseResizeCurrentConfiguration.test.tsx

@@ -63,7 +63,7 @@ describe('database current configuration section', () => {
    getByText('1 GB');

    getByText('CPUs');
-    getByText('2');
+    getByText('4');


because of databaseTypeFactory:

vcpus: Factory.each((i) => i * 2)

abailly-akamai · 2024-06-27T02:49:55Z

packages/manager/src/features/Kubernetes/KubeCheckoutBar/KubeCheckoutBar.test.tsx

@@ -118,7 +168,7 @@ describe('KubeCheckoutBar', () => {
    );

    // 5 node pools * 3 linodes per pool * 12 per linode * 20% increase for Jakarta + UNKNOWN_PRICE
-    await findByText(/\$180\.00/);
+    await findByText(/\$183\.00/);


no entirely sure about how the prices are calculated but considering id: 0 k8 type may not have been taken into consideration before i assume this is right.

abailly-akamai · 2024-06-27T02:50:35Z

...ager/src/features/Linodes/LinodeCreatev2/Tabs/StackScripts/StackScriptDetailsDialog.test.tsx

-    const stackscript = stackScriptFactory.build();
+    const stackscript = stackScriptFactory.build({
+      id: 1234,
+    });


avoiding multiple 1 assertions on the page

abailly-akamai · 2024-06-27T03:06:31Z

packages/manager/src/components/EnhancedNumberInput/EnhancedNumberInput.tsx

    margin: 0,
  },
-  '-moz-appearance': 'textfield',
+  MozAppearance: 'textfield',


unrelated to PR but was really tired to see this warning in the logs 🧹

This has annoyed me forever too! 🙌

abailly-akamai · 2024-06-27T03:07:14Z

packages/manager/src/features/Images/ImageSelect.test.tsx

@@ -74,8 +77,9 @@ describe('ImageSelect', () => {
  describe('ImageSelect component', () => {
    it('should render', () => {
      const { getByText } = renderWithTheme(<ImageSelect {...props} />);
-      getByText(/image-0/i);
+      getByText(/image-1(?!\d)/i);


avoid matching image-10

github-actions · 2024-06-27T03:57:15Z

Coverage Report: ✅
Base Coverage: 82.24%
Current Coverage: 82.24%

abailly-akamai · 2024-06-27T04:40:17Z

packages/manager/src/factories/factoryOverride.ts

+    },
+    writable: false,
+  });
+}


@jdamore-linode this unblocks the issues we'll encounter with MSW however cypress is rather unhappy with overriding Factory.each.

We have a few options:

We find a way for cypress to accept the override (if you remove this check you'll see every test with a factory fail)

we go away from overriding the property and go a route of wrapping our factories in an HOC or use a custom Each for every factory or just add +1 everywhere, which I wanted to avoid

we leave as is and keep this change out of cypress
Thoughts?

@jdamore-linode please see 886d981 and let me know what you think! I think this is clear and elegant enough to satisfy the requirements and have cypress benefit from this update

dwiley-akamai

Tests pass in CI (I did observe a handful of failures locally) ✅

packages/manager/.changeset/pr-10619-tech-stories-1719459021760.md

packages/manager/src/factories/factoryOverride.ts

mjac0bs

Looks good overall! I just saw and noted one case where we changed the expected output of the test instead of updated an id.

Unit tests passed locally and CI looks good.

packages/manager/src/features/Linodes/LinodesLanding/utils.test.ts

packages/manager/vite.config.ts

jdamore-linode

Nice, thanks for taking this on @abailly-akamai! Approved pending unit tests passing (seems there's currently a GHA incident which I'm assuming is the reason those jobs haven't run)

packages/manager/tsconfig.json

abailly-akamai added 4 commits June 26, 2024 14:18

Initial commit - save work

19005a8

save progress

00143bc

save progress

4585022

last one

1d472a3

abailly-akamai added the Tech Debt label Jun 26, 2024

abailly-akamai self-assigned this Jun 26, 2024

abailly-akamai added 2 commits June 26, 2024 17:58

comment and cleanup

ad1a277

cleanup

c1380fa

abailly-akamai commented Jun 27, 2024

View reviewed changes

abailly-akamai marked this pull request as ready for review June 27, 2024 03:28

abailly-akamai requested a review from a team as a code owner June 27, 2024 03:28

abailly-akamai requested review from dwiley-akamai and mjac0bs and removed request for a team June 27, 2024 03:28

Added changeset: Enforce Factory.each to start increment at 1

73f7d60

abailly-akamai added 2 commits June 27, 2024 00:32

exclude cypress for now

5a7f5c5

comment

24af6f9

abailly-akamai requested a review from jdamore-linode June 27, 2024 04:35

abailly-akamai commented Jun 27, 2024

View reviewed changes

dwiley-akamai reviewed Jun 27, 2024

View reviewed changes

packages/manager/.changeset/pr-10619-tech-stories-1719459021760.md Outdated Show resolved Hide resolved

packages/manager/src/factories/factoryOverride.ts Outdated Show resolved Hide resolved

mjac0bs approved these changes Jun 27, 2024

View reviewed changes

packages/manager/src/features/Linodes/LinodesLanding/utils.test.ts Outdated Show resolved Hide resolved

abailly-akamai added 2 commits June 28, 2024 13:36

Going the proxy route in order to satisfy cypress

886d981

feedback @dwiley-akamai @mjac0bs

f3016ae

abailly-akamai requested a review from a team as a code owner June 28, 2024 17:42

abailly-akamai requested review from AzureLatte and removed request for a team June 28, 2024 17:42

bnussman-akamai reviewed Jun 28, 2024

View reviewed changes

packages/manager/vite.config.ts Outdated Show resolved Hide resolved

Joe D'Amore and others added 2 commits June 28, 2024 15:00

Remove hard-coded plan type labels in test assertions

4805107

feedback @bnussman-akamai

c3c8beb

jdamore-linode approved these changes Jun 28, 2024

View reviewed changes

small cleanup

c894427

bnussman-akamai approved these changes Jun 28, 2024

View reviewed changes

packages/manager/tsconfig.json Outdated Show resolved Hide resolved

dwiley-akamai approved these changes Jun 28, 2024

View reviewed changes

abailly-akamai merged commit e64101c into linode:develop Jun 29, 2024
18 checks passed

This was referenced Jul 8, 2024

Release v1.123.0 - release → staging #10648

Merged

Release v1.123.0 - staging → master #10652

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: [M3-8291] - Enforce Factory.each to start increment at `1` #10619

refactor: [M3-8291] - Enforce Factory.each to start increment at `1` #10619

abailly-akamai commented Jun 26, 2024 •

edited

Loading

abailly-akamai Jun 27, 2024

abailly-akamai Jun 27, 2024

abailly-akamai Jun 27, 2024

abailly-akamai Jun 27, 2024

jdamore-linode Jun 27, 2024

abailly-akamai Jun 27, 2024

github-actions bot commented Jun 27, 2024 •

edited

Loading

abailly-akamai Jun 27, 2024 •

edited

Loading

abailly-akamai Jun 28, 2024

dwiley-akamai left a comment

mjac0bs left a comment

jdamore-linode left a comment

refactor: [M3-8291] - Enforce Factory.each to start increment at 1 #10619

refactor: [M3-8291] - Enforce Factory.each to start increment at 1 #10619

Conversation

abailly-akamai commented Jun 26, 2024 • edited Loading

Description 📝

Changes 🔄

How to test 🧪

Verification steps

As an Author I have considered 🤔

abailly-akamai Jun 27, 2024

Choose a reason for hiding this comment

abailly-akamai Jun 27, 2024

Choose a reason for hiding this comment

abailly-akamai Jun 27, 2024

Choose a reason for hiding this comment

abailly-akamai Jun 27, 2024

Choose a reason for hiding this comment

jdamore-linode Jun 27, 2024

Choose a reason for hiding this comment

abailly-akamai Jun 27, 2024

Choose a reason for hiding this comment

github-actions bot commented Jun 27, 2024 • edited Loading

abailly-akamai Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

abailly-akamai Jun 28, 2024

Choose a reason for hiding this comment

dwiley-akamai left a comment

Choose a reason for hiding this comment

mjac0bs left a comment

Choose a reason for hiding this comment

jdamore-linode left a comment

Choose a reason for hiding this comment

refactor: [M3-8291] - Enforce Factory.each to start increment at `1` #10619

refactor: [M3-8291] - Enforce Factory.each to start increment at `1` #10619

abailly-akamai commented Jun 26, 2024 •

edited

Loading

github-actions bot commented Jun 27, 2024 •

edited

Loading

abailly-akamai Jun 27, 2024 •

edited

Loading