Adding static icon names to test env file #16078

georgewrmarshall · 2022-10-04T21:42:55Z

Explanation

Currently, any unit tests that use the <Icon /> component will receive a propType error because ICON_NAMES environment variable doesn't exist in the testing environment.

This is a problem because it makes it very hard to unit test components using Icons

In order to solve this problem, we are changing the environment variable ICON_NAMES from an object to a string. We can then parse it on the component end. It also duplicates the ICON_NAMES env variable in env.js so icon names will be available in the testing environment.

The genius that is @Gudahtt figured out that we had to update the generateIconNames function to synchronous for it to work so this PR also does just that.

More Information

Fixes Find work around for ICON_NAMES environment vars for tests #16046

Manual Testing Steps

The unit test apart of this PR should be sufficient

Pre-Merge Checklist

PR template is filled out
IF this PR fixes a bug, a test that would have caught the bug has been added
PR is linked to the appropriate GitHub issue
PR has been added to the appropriate release Milestone

+ If there are functional changes:

Manual testing complete & passed
"Extension QA Board" label has been applied

github-actions · 2022-10-04T21:43:05Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

georgewrmarshall · 2022-10-04T22:13:14Z

Hey @Gudahtt I see what you mean about this being tricky with stringifying the env variable. Is there any way of conditionally setting the ICON_NAMES constant in ui/components/component-library/icon/icon.constants.js just for tests? I guess I'm struggling on 2 parts:

how to ensure the tests are receiving the correct ICON_NAMEs type
how we can ensure the test env vars don't become out of sync with the production vars. Should I use a script to update the env.js file and can that be done in development/generate-icon-names.js?

Gudahtt · 2022-10-13T17:14:20Z

So the approach we've taken before is to make the environment variable a string all of the time, even in prod. We can use JSON.parse to convert it to an object. That way the tests can set the environment variable string, and things work correctly both in prod and in the unit tests

georgewrmarshall · 2022-10-27T14:06:03Z

ui/components/component-library/icon/icon.test.js

-  BOOKMARK_FILLED: 'bookmark-filled',
-  CALCULATOR_FILLED: 'calculator-filled',
-};
-


No longer need to mock the icon names env var because we are setting it in test/env.js

test/env.js

georgewrmarshall · 2022-10-27T14:07:24Z

app/images/icons/icon-user-cirlce-add-filled.svg

@@ -1,3 +0,0 @@
-<svg xmlns="http://www.w3.org/2000/svg" id="icon-user-cirlce-add-filled" viewBox="0 0 512 512">
-  <path d="m448 70c-14-17-36-27-60-27-23 0-43 9-58 24-9 9-15 20-19 31-3 8-4 17-4 26 0 15 4 30 12 42 4 7 9 13 15 18 14 13 33 21 54 21 9 0 18-1 25-4 19-6 35-19 45-35 4-7 7-15 9-23 2-6 2-13 2-19 0-21-8-40-21-54z m-30 69l-15 0 0 16c0 8-7 15-15 15-8 0-15-7-15-15l0-16-15 0c-9 0-15-7-15-15 0-9 6-16 15-16l15 0 0-14c0-8 7-15 15-15 8 0 15 7 15 15l0 14 15 0c9 0 16 7 16 16 0 8-7 15-16 15z m31 127c0-26-5-52-15-75-6 4-13 8-21 10-2 1-4 1-6 2 7 19 12 41 12 63 0 47-20 90-50 121-6-7-14-14-23-20-55-37-145-37-201 0-8 6-16 13-22 20-31-31-50-74-50-121 0-95 78-173 173-173 22 0 43 5 63 12 0-2 1-4 2-7 2-7 6-14 10-20-23-10-49-15-75-15-112 0-203 91-203 203 0 59 25 112 65 149 0 0 0 0 0 1 2 2 5 3 7 5 1 1 2 2 3 3 4 3 8 6 12 9 1 1 2 2 4 3 4 2 8 5 12 7 1 1 3 2 4 3 4 2 9 4 13 6 2 0 3 1 5 2 4 2 9 3 13 5 2 0 4 1 5 1 5 2 10 3 15 4 1 1 3 1 4 1 6 1 12 2 18 3 0 0 1 0 2 0 7 1 14 1 21 1 7 0 14 0 20-1 1 0 2 0 3 0 6-1 11-2 17-3 1 0 3-1 5-1 4-1 9-2 14-4 2 0 3-1 5-1 5-2 9-3 13-5 2-1 4-2 5-2 5-2 9-4 13-6 2-1 3-2 5-3 4-3 8-5 12-7 1-1 2-2 4-3 4-3 8-6 11-9 2-1 3-2 4-3 2-2 4-3 6-5 0-1 0-1 0-1 41-37 66-90 66-149z m-203-103c-42 0-76 34-76 76 0 42 32 75 75 76 0 0 1 0 2 0 0 0 1 0 1 0 0 0 0 0 0 0 42-1 74-34 74-76 0-42-34-76-76-76z"/>
-</svg>


Removing duplicate icon

metamaskbot · 2022-10-27T15:13:06Z

Builds ready [d9b8bec]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2515 ± 320 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	90	2294	347	525	252
		domContentLoaded	1715	4342	2501	669	321
		load	1715	4342	2515	665	320
		domInteractive	1715	4342	2501	669	321

highlights:

storybook

georgewrmarshall · 2022-11-07T21:10:59Z

development/generate-icon-names.js

-  return iconNames;
+  const iconNamesStringified = JSON.stringify(iconNames);
+
+  return iconNamesStringified;
 };

 module.exports = { generateIconNames };


Updating generateIconNames to be synchronous because the genius that is @Gudahtt somehow figured out that to get it to work with jest

I was able to get it to work by making generateIconNames synchronous (swapping out fs.promises.readdir with fs.readdirSync), and calling it in test.env.js. Must be some Jest bug/quirk about async setup, I don't understand what's going on there,

test/env.js

Gudahtt

LGTM!

Edit: Whoops, mocha test failures now.

…ponents

georgewrmarshall · 2022-11-18T00:08:16Z

ui/components/component-library/picker-network/__snapshots__/picker-network.test.js.snap

@@ -18,7 +18,7 @@ exports[`PickerNetwork should render the label inside the PickerNetwork 1`] = `
    </p>
    <div
      class="box mm-picker-network__arrow-down-icon icon icon--size-xs box--flex-direction-row box--color-icon-default"
-      style="mask-image: url('./images/icons/icon-undefined.svg;"
+      style="mask-image: url('./images/icons/icon-arrow-down.svg;"
    />
  </button>
 </div>


Having to update snapshots means that it's working!

Still need to get the url formatting fixed

style="mask-image: url('./images/icons/icon-arrow-down.svg;')"

Yup! It will be apart of the house keeping ticket

georgewrmarshall · 2022-11-18T00:09:12Z

test/env.js

-process.env.ICON_NAMES = {
-  LOADING_FILLED: 'loading-filled',
-  CLOSE_OUTLINE: 'close-outline',
-};


These are now taken care of by generateIconNames in test/jest/env.js below

metamaskbot · 2022-11-18T01:20:20Z

Builds ready [46a5e7c]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (2070 ± 97 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	95	121	104	6	3
		domContentLoaded	1702	2481	2044	208	100
		load	1713	2509	2070	202	97
		domInteractive	1702	2481	2044	208	100

Bundle size diffs

background: 0 bytes
ui: 224 bytes
common: 0 bytes

highlights:

storybook

Gudahtt

LGTM!

georgewrmarshall · 2022-11-18T01:26:58Z

Might also be worth mentioning that there is another reference to env.js but its not used and I have a PR to remove this config file completely #16577

georgewrmarshall changed the title ~~Adding static icon names to test env file~~ Adding static icon names to test env file (WIP) Oct 4, 2022

georgewrmarshall added the DO-NOT-MERGE Pull requests that should not be merged label Oct 4, 2022

georgewrmarshall self-assigned this Oct 4, 2022

georgewrmarshall force-pushed the fix/16046/icon-name-test-env-vars branch from bdcdd2e to 8098177 Compare October 13, 2022 17:10

georgewrmarshall mentioned this pull request Oct 13, 2022

Adding TextField component #16105

Merged

6 tasks

georgewrmarshall mentioned this pull request Oct 27, 2022

Feat/15088/add button icon #16277

Merged

6 tasks

georgewrmarshall force-pushed the fix/16046/icon-name-test-env-vars branch from 8098177 to d9b8bec Compare October 27, 2022 14:04

georgewrmarshall marked this pull request as ready for review October 27, 2022 14:04

georgewrmarshall requested review from a team and kumavis as code owners October 27, 2022 14:04

georgewrmarshall requested review from PeterYinusa and Gudahtt and removed request for PeterYinusa October 27, 2022 14:04

georgewrmarshall removed the DO-NOT-MERGE Pull requests that should not be merged label Oct 27, 2022

georgewrmarshall commented Oct 27, 2022

View reviewed changes

test/env.js Outdated Show resolved Hide resolved

georgewrmarshall commented Oct 27, 2022

View reviewed changes

georgewrmarshall requested review from garrettbear and NidhiKJha October 27, 2022 14:12

georgewrmarshall force-pushed the fix/16046/icon-name-test-env-vars branch from d9b8bec to 71bcede Compare November 7, 2022 21:08

georgewrmarshall commented Nov 7, 2022

View reviewed changes

test/env.js Outdated Show resolved Hide resolved

Gudahtt previously approved these changes Nov 7, 2022

View reviewed changes

georgewrmarshall marked this pull request as draft November 7, 2022 21:56

garrettbear requested a review from Gudahtt November 8, 2022 00:13

georgewrmarshall mentioned this pull request Nov 10, 2022

TextField component updates #16424

Merged

8 tasks

georgewrmarshall added 3 commits November 17, 2022 17:48

Storing the icon name env var as a string and parsing to use with com…

69b78a5

…ponents

Moving icon env vars to jest specific env.js file

167b311

Updating snapshots

46a5e7c

georgewrmarshall dismissed Gudahtt’s stale review via 46a5e7c November 17, 2022 23:57

georgewrmarshall force-pushed the fix/16046/icon-name-test-env-vars branch from 71bcede to 46a5e7c Compare November 17, 2022 23:57

georgewrmarshall commented Nov 18, 2022

View reviewed changes

georgewrmarshall marked this pull request as ready for review November 18, 2022 00:26

Gudahtt approved these changes Nov 18, 2022

View reviewed changes

Gudahtt changed the title ~~Adding static icon names to test env file (WIP)~~ Adding static icon names to test env file Nov 18, 2022

garrettbear approved these changes Nov 18, 2022

View reviewed changes

NidhiKJha merged commit 3af83f8 into develop Nov 18, 2022

NidhiKJha deleted the fix/16046/icon-name-test-env-vars branch November 18, 2022 16:58

github-actions bot locked and limited conversation to collaborators Nov 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding static icon names to test env file #16078

Adding static icon names to test env file #16078

georgewrmarshall commented Oct 4, 2022 •

edited

Loading

github-actions bot commented Oct 4, 2022

georgewrmarshall commented Oct 4, 2022

Gudahtt commented Oct 13, 2022

georgewrmarshall Oct 27, 2022

georgewrmarshall Oct 27, 2022

metamaskbot commented Oct 27, 2022

georgewrmarshall Nov 7, 2022 •

edited

Loading

Gudahtt left a comment •

edited

Loading

georgewrmarshall Nov 18, 2022

garrettbear Nov 18, 2022

georgewrmarshall Nov 18, 2022

georgewrmarshall Nov 18, 2022

metamaskbot commented Nov 18, 2022

Gudahtt left a comment

georgewrmarshall commented Nov 18, 2022 •

edited

Loading

Adding static icon names to test env file #16078

Adding static icon names to test env file #16078

Conversation

georgewrmarshall commented Oct 4, 2022 • edited Loading

Explanation

More Information

Manual Testing Steps

Pre-Merge Checklist

+ If there are functional changes:

github-actions bot commented Oct 4, 2022

georgewrmarshall commented Oct 4, 2022

Gudahtt commented Oct 13, 2022

georgewrmarshall Oct 27, 2022

Choose a reason for hiding this comment

georgewrmarshall Oct 27, 2022

Choose a reason for hiding this comment

metamaskbot commented Oct 27, 2022

highlights:

georgewrmarshall Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Gudahtt left a comment • edited Loading

Choose a reason for hiding this comment

georgewrmarshall Nov 18, 2022

Choose a reason for hiding this comment

garrettbear Nov 18, 2022

Choose a reason for hiding this comment

georgewrmarshall Nov 18, 2022

Choose a reason for hiding this comment

georgewrmarshall Nov 18, 2022

Choose a reason for hiding this comment

metamaskbot commented Nov 18, 2022

highlights:

Gudahtt left a comment

Choose a reason for hiding this comment

georgewrmarshall commented Nov 18, 2022 • edited Loading

georgewrmarshall commented Oct 4, 2022 •

edited

Loading

georgewrmarshall Nov 7, 2022 •

edited

Loading

Gudahtt left a comment •

edited

Loading

georgewrmarshall commented Nov 18, 2022 •

edited

Loading