Add build-time code exclusion using code fencing #12060

rekmarks · 2021-09-10T01:11:16Z

This PR adds build-time code exclusion by means of code fencing. For details, please see the README. Note that linting of transformed files as a form of validation is added in a follow-up, #12075.

Hopefully exhaustive tests are added to ensure that the transform works according to its specification. Since these tests are Node-only, they required their own Jest config. The recommended way to work with multiple Jest configs is using the projects field in the Jest config, however that feature breaks coverage collection. That being the case, I had to set up two separate Jest configs. In order to get both test suites to run in parallel, Jest is now invoked via a script, ./test/run-jest.sh.

By way of example, this build system feature allows us to add fences like this:

this.store.updateStructure({
  ...,
  GasFeeController: this.gasFeeController,
  TokenListController: this.tokenListController,
  ///: BEGIN:ONLY_INCLUDE_IN(beta)
  PluginController: this.pluginController,
  ///: END:ONLY_INCLUDE_IN
});

Which at build time are transformed to the following if the build type is not beta:

this.store.updateStructure({
  ...,
  GasFeeController: this.gasFeeController,
  TokenListController: this.tokenListController,
});

kumavis

use the example luke 🧙‍♂️

development/build/transforms/code-fencing.js

kumavis · 2021-09-10T01:25:29Z

generally good

metamaskbot · 2021-09-10T01:26:04Z

Builds ready [6d9512e]

builds: chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (371 ± 18 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	54	508	335	100	48
		domContentLoaded	326	494	356	37	18
		load	342	511	371	37	18
		domInteractive	326	494	356	37	18

metamaskbot · 2021-09-10T16:55:29Z

Builds ready [6f97fa6]

builds: chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (381 ± 20 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	51	431	337	96	46
		domContentLoaded	329	492	363	38	18
		load	345	518	381	41	20
		domInteractive	329	491	363	38	18

metamaskbot · 2021-09-10T22:30:36Z

Builds ready [ba99fcd]

builds: chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (379 ± 20 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	46	513	282	151	72
		domContentLoaded	327	502	365	42	20
		load	341	511	379	41	20
		domInteractive	327	502	365	42	20

metamaskbot · 2021-09-11T16:50:06Z

Builds ready [7f2e40a]

builds: chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (416 ± 28 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	60	495	346	124	60
		domContentLoaded	338	595	400	58	28
		load	353	603	416	58	28
		domInteractive	338	595	400	58	28

metamaskbot · 2021-09-12T05:24:47Z

Builds ready [f74378d]

builds: chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (404 ± 21 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	55	506	364	109	52
		domContentLoaded	337	494	386	40	19
		load	352	530	404	43	21
		domInteractive	337	494	386	40	19

metamaskbot · 2021-09-13T03:42:06Z

Builds ready [b0c42ec]

builds: chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (361 ± 18 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	48	493	340	77	37
		domContentLoaded	320	479	345	36	17
		load	334	488	361	37	18
		domInteractive	320	479	345	36	17

darkwing

This looks really good and my testing was successful. Well done, and very quickly at that!

darkwing · 2021-09-13T17:16:52Z

jest.config.js

-    '<rootDir>/shared/**/?(*.)+(test).js',
-  ],
+  // TODO: enable resetMocks
+  // resetMocks: true,


Will this take place in a later update?

resetMocks was already not enabled, and enabling it caused errors. I'm just highlighting that we should enable it here!

development/build/transforms/remove-fenced-code.js

kumavis

lgtm 🔥

development/build/transforms/remove-fenced-code.js

Gudahtt · 2021-09-13T21:12:45Z

jest.config.js

+  setupFiles: ['<rootDir>/test/setup.js', '<rootDir>/test/env.js'],
+  setupFilesAfterEnv: ['<rootDir>/test/jest/setup.js'],
+  testMatch: ['<rootDir>/ui/**/*.test.js', '<rootDir>/shared/**/*.test.js'],
+  testTimeout: 2500,


This looks new - is this the default setting?

We bumped the timeout at one point in the module template: https://github.com/MetaMask/metamask-module-template/blob/main/jest.config.js#L26

test/run-jest.sh

Gudahtt · 2021-09-13T21:23:22Z

test/run-jest.sh

+# For storing PIDs of subshells
+PIDS=(0 0)
+
+yarn 'jest' '--config=./jest.config.js' "$@" & PIDS[0]=$!


This pattern seems questionable 🤔 This means if one process fails, the other will keep going and there will be no way to stop it. Sounds like a pain.

We have a package that is supposed to do this sort of thing for us: concurrently. I don't think it works correctly in all cases, but it might work more often than this.

I think we could scrap this whole script and call concurrently directly from the npm script.

This means if one process fails, the other will keep going and there will be no way to stop it. Sounds like a pain.

This is an intentional feature, not a bug! The idea being that, in CI, you want to know of all unit test failures, not just the ones for a particular config. This is an attempt to replicate what the Jest projects feature does, which is to run all tests for every project and then print the complete results.

If developing locally, the tests for a particular config can be run by doing yarn jest --config=PATH_TO_CONFIG.

On the topic of using concurrently, that seems like a good idea, since it appears to be a well-maintained tool that does exactly what I'm trying to do. We may still have to keep this script in order to do something like yarn test:unit:jest --watch and pass the --watch argument to both invocations of jest. That's what the "$@" is about in the Bash script.

Just to be clear, when I say "there is no way to stop it", what I mean is that the interrupt signal no longer halts the script. I certainly hope disabling CTRL+C wasn't intentional! It's incredibly frustrating when scripts can't be halted.

... I tested this, and it works 🤔 I guess it's because these tests don't spawn child processes? This was a struggle to get working with the e2e tests. Our attempts to combine tests just with bash resulted in them continuing on after interrupting the main process.

I decided to go with concurrently but keep the bash script. I went for concurrently because a tool we're already using is probably better than a weird Bash loop-and-wait implementation. However, I invoke concurrently in the .sh file to preserve the ability to pass arguments to both jest processes by doing e.g. yarn test:unit:jest --silent.

662f5ae

metamaskbot · 2021-09-13T23:04:16Z

Builds ready [0828289]

builds: chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (375 ± 17 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	53	436	303	125	60
		domContentLoaded	324	473	360	35	17
		load	339	481	375	36	17
		domInteractive	324	473	360	35	17

development/build/transforms/remove-fenced-code.js

metamaskbot · 2021-09-14T00:21:55Z

Builds ready [073f7cf]

builds: chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (371 ± 10 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	53	415	334	95	46
		domContentLoaded	331	398	355	17	8
		load	347	434	371	20	10
		domInteractive	331	398	355	17	8

development/build/transforms/remove-fenced-code.js

development/build/transforms/remove-fenced-code.test.js

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

rekmarks · 2021-09-14T06:14:20Z

892a0a1 fixes a RegEx-related bug introduced in ce15bf2, and a completely different bug discovered while trying to fix the RegEx bug.

The RegEx bug was due to (what seems to me) somewhat strange behavior by the JavaScript RegEx engine (or just the spec, who knows), in that /s*\w+/gmu will match '\nstuff\n' as the single line '\nstuff', as opposed the two lines '' and stuff that we might expect. The RegEx had to be modified to avoid this and still produce the desired effect.

While writing tests to ensure that the parser indeed ignores sentinels preceded by non-whitespace characters (i.e. what ce15bf2 was supposed to accomplish), I discovered that the multiSplice function didn't handle the case where splicingIndices.length === 2. Specifically, it would insert the last retained part of the original string twice. This has now been fixed.

metamaskbot · 2021-09-14T06:19:09Z

Builds ready [892a0a1]

builds: chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7, 8
- common: 0, 1, 2, 3
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- sentry-install: sentry-install
- ui: 0, 1, 2, 3, 4, 5

Page Load Metrics (378 ± 24 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	45	485	336	97	47
		domContentLoaded	331	511	363	51	25
		load	345	513	378	50	24
		domInteractive	331	511	363	51	25

Gudahtt

LGTM!

brad-decker

This is great. Good work. Tested locally and it checks out.

Gudahtt · 2021-09-14T14:46:12Z

development/build/transforms/README.md

@@ -0,0 +1,123 @@
+# Local Browserify Transforms


This is some incredible documentation by the way. Not just this but the inline docs too.

I was expecting this transform to be difficult to understand, but it was really straightforward with all of these explanations at each step.

Initial code fencing transform implementation

6d9512e

kumavis requested changes Sep 10, 2021

View reviewed changes

development/build/transforms/code-fencing.js Outdated Show resolved Hide resolved

rekmarks added 2 commits September 10, 2021 00:19

Rename some stuff

779974a

Concatenate file buffers before applying transform

6f97fa6

rekmarks force-pushed the code-fencing branch 2 times, most recently from bd95c35 to 7f2e40a Compare September 11, 2021 08:23

rekmarks added 3 commits September 11, 2021 09:41

Begin adding tests

ac3d520

Complete tests, reconfigure Jest

3e7fbdb

Rename fileName parameter to filePath

6cdb3a8

Lint transformed files, init lint tests

f6126d1

rekmarks force-pushed the code-fencing branch from 7f2e40a to f6126d1 Compare September 11, 2021 22:10

rekmarks added 7 commits September 11, 2021 18:24

Finish transformed linting tests

28c649d

Add spaced-comment marker for fences

a90fb76

Add build system arg for toggling fence linting

999250c

Replace through2 with a Transform implementation

20bccdd

Default shouldLintFenceFiles to false for dev builds

2622421

Fix spaced-comment config

c41078a

Update LavaMoat policy

a950ea2

rekmarks force-pushed the code-fencing branch from 4af5572 to a950ea2 Compare September 12, 2021 04:55

fixup! Update LavaMoat policy

09ca404

rekmarks force-pushed the code-fencing branch from ec6d0cf to 09ca404 Compare September 12, 2021 04:57

Check that the linter is called correctly in tests

f74378d

rekmarks added 2 commits September 12, 2021 10:57

Add transform function docstrings

5bd2a12

Add some directive test cases

bde0241

Tweak a test

b0c42ec

darkwing previously approved these changes Sep 13, 2021

View reviewed changes

rekmarks dismissed darkwing’s stale review via cb1f59a September 13, 2021 18:49

kumavis reviewed Sep 13, 2021

View reviewed changes

development/build/transforms/remove-fenced-code.js Outdated Show resolved Hide resolved

Stop using 'key in object'

efd8cd3

rekmarks force-pushed the code-fencing branch from cb1f59a to efd8cd3 Compare September 13, 2021 18:54

Address review feedback

0828289

kumavis previously approved these changes Sep 13, 2021

View reviewed changes

development/build/transforms/remove-fenced-code.js Outdated Show resolved Hide resolved

brad-decker dismissed kumavis’s stale review via 0828289 September 13, 2021 21:01

Gudahtt reviewed Sep 13, 2021

View reviewed changes

Fix SC2048 violation

073f7cf

Gudahtt reviewed Sep 14, 2021

View reviewed changes

development/build/transforms/remove-fenced-code.js Outdated Show resolved Hide resolved

development/build/transforms/remove-fenced-code.js Outdated Show resolved Hide resolved

rekmarks added 2 commits September 13, 2021 20:20

Update development/build/transforms/remove-fenced-code.js

dfadb27

Update development/build/transforms/remove-fenced-code.js

ce15bf2

Gudahtt reviewed Sep 14, 2021

View reviewed changes

development/build/transforms/remove-fenced-code.js Outdated Show resolved Hide resolved

development/build/transforms/remove-fenced-code.test.js Show resolved Hide resolved

rekmarks and others added 3 commits September 13, 2021 21:59

Apply suggestions from code review

474ec02

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Fix test scripts

662f5ae

Fix tests and some bugs

892a0a1

rekmarks requested a review from Gudahtt September 14, 2021 06:14

Gudahtt approved these changes Sep 14, 2021

View reviewed changes

brad-decker approved these changes Sep 14, 2021

View reviewed changes

Gudahtt reviewed Sep 14, 2021

View reviewed changes

rekmarks merged commit 3de3765 into develop Sep 14, 2021

rekmarks deleted the code-fencing branch September 14, 2021 17:00

github-actions bot locked and limited conversation to collaborators Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build-time code exclusion using code fencing #12060

Add build-time code exclusion using code fencing #12060

rekmarks commented Sep 10, 2021 •

edited

Loading

kumavis left a comment

kumavis commented Sep 10, 2021

metamaskbot commented Sep 10, 2021

metamaskbot commented Sep 10, 2021

metamaskbot commented Sep 10, 2021

metamaskbot commented Sep 11, 2021

metamaskbot commented Sep 12, 2021

metamaskbot commented Sep 13, 2021

darkwing left a comment

darkwing Sep 13, 2021

rekmarks Sep 13, 2021

kumavis left a comment

Gudahtt Sep 13, 2021

rekmarks Sep 13, 2021

Gudahtt Sep 13, 2021

rekmarks Sep 13, 2021

Gudahtt Sep 14, 2021

Gudahtt Sep 14, 2021 •

edited

Loading

rekmarks Sep 14, 2021 •

edited

Loading

metamaskbot commented Sep 13, 2021

metamaskbot commented Sep 14, 2021

rekmarks commented Sep 14, 2021 •

edited

Loading

metamaskbot commented Sep 14, 2021

Gudahtt left a comment

brad-decker left a comment

Gudahtt Sep 14, 2021

brad-decker Sep 14, 2021

Add build-time code exclusion using code fencing #12060

Add build-time code exclusion using code fencing #12060

Conversation

rekmarks commented Sep 10, 2021 • edited Loading

kumavis left a comment

Choose a reason for hiding this comment

kumavis commented Sep 10, 2021

metamaskbot commented Sep 10, 2021

metamaskbot commented Sep 10, 2021

metamaskbot commented Sep 10, 2021

metamaskbot commented Sep 11, 2021

metamaskbot commented Sep 12, 2021

metamaskbot commented Sep 13, 2021

darkwing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumavis left a 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

Gudahtt Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

rekmarks Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

metamaskbot commented Sep 13, 2021

metamaskbot commented Sep 14, 2021

rekmarks commented Sep 14, 2021 • edited Loading

metamaskbot commented Sep 14, 2021

Gudahtt left a comment

Choose a reason for hiding this comment

brad-decker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rekmarks commented Sep 10, 2021 •

edited

Loading

Gudahtt Sep 14, 2021 •

edited

Loading

rekmarks Sep 14, 2021 •

edited

Loading

rekmarks commented Sep 14, 2021 •

edited

Loading