Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🏗 Replace browserify in tests with esbuild #32891

Merged
merged 32 commits into from
Mar 2, 2021
Merged

🏗 Replace browserify in tests with esbuild #32891

merged 32 commits into from
Mar 2, 2021

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Feb 24, 2021

This is another in a series of PRs that modernize our development tasks.

PR highlights:

  • Replace karma-browserify with karma-esbuild for test transforms
  • Replace browserify and babelify in tests with esbuild and babel
  • Delete babelify, browserify, browserify-persist-fs, karma-browserify from package.json
  • Necessary changes to get tests to run and pass:
    • Remove the --new_server flag and always build transforms before server start up
    • Perform esbuild transforms for JS files in one go on a unified JS test file
    • Serve a test script for test-3p.js (because Karma now serves the unified file instead of individual test files)
    • Make some ES6 modules use import instead of require
    • Replace global object with its platform-specific equivalent
    • Downconvert esbuild output to ES5 on Windows so tests can run on IE 11
  • Accompanying cleanup:
    • Remove persistent Karma browserify cache (to be replaced later by one for esbuild)
    • Clean up karma.conf.js and move dynamic config to runtime-test-base.js
    • Consolidate esbuild config computation for all tasks into one helper function
    • Remove unnecessary server building logic from integration.js
    • Add missing await calls to racy test status reporting
    • Fix scripts in third_party/ that were using browserify, etc. to invoke via npx

Partial fix for #32585

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a few fixes.

extensions/amp-date-picker/0.1/dates-list.js Outdated Show resolved Hide resolved
build-system/tasks/helpers.js Show resolved Hide resolved
build-system/tasks/helpers.js Outdated Show resolved Hide resolved
Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great ⭐

build-system/tasks/runtime-test/runtime-test-base.js Outdated Show resolved Hide resolved
build-system/tasks/dep-check.js Show resolved Hide resolved
build-system/tasks/helpers.js Show resolved Hide resolved
@rsimha rsimha removed the request for review from dvoytenko February 27, 2021 06:58
@rsimha
Copy link
Contributor Author

rsimha commented Feb 27, 2021

All green! Ready for full review from @jridgewell, @samouri, @erwinmombay.

@rsimha rsimha removed the request for review from kristoferbaxter March 1, 2021 18:29
Copy link
Member

@samouri samouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of comments, stil working through the last few files

build-system/babel-config/helpers.js Outdated Show resolved Hide resolved
build-system/babel-config/helpers.js Outdated Show resolved Hide resolved
build-system/tasks/dep-check.js Show resolved Hide resolved
build-system/tasks/dep-check.js Show resolved Hide resolved
build-system/tasks/helpers.js Show resolved Hide resolved
build-system/tasks/runtime-test/helpers.js Show resolved Hide resolved
build-system/tasks/runtime-test/runtime-test-base.js Outdated Show resolved Hide resolved
build-system/babel-config/helpers.js Outdated Show resolved Hide resolved
build-system/server/app.js Show resolved Hide resolved
build-system/tasks/runtime-test/runtime-test-base.js Outdated Show resolved Hide resolved
build-system/tasks/runtime-test/runtime-test-base.js Outdated Show resolved Hide resolved

if (argv.coverage) {
config.reporters.push('coverage-istanbul');
const isJsGlob = (glob) => typeof glob === 'string' && glob.endsWith('.js');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dependent on us using .js as the only ending token, but any change to use more advanced globby features could break that. Eg, future support for TS and changing the glob to .{js,ts}. I'd rather this be explicit about fixing esbuild generation by looking for keys with values of ['esbuild', ...].

Copy link
Contributor Author

@rsimha rsimha Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintaining two lists, one for which files to include in the Karma server, and one for which files to run transforms on is unnecessary, because we transform every JS file with esbuild (that was the original intent). However, I see your point that not every JS glob must end with JS. One way to deal with this is to expand all the globs before processing. Will do this.

Copy link
Contributor Author

@rsimha rsimha Mar 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, I now see why we needed two lists. There are JS files in examples/ that shouldn't be transformed. I stand corrected. I've modified the logic to expand globs, but still automatically detect which files we want to transform with esbuild and which ones we don't. Lemme know what you think.

Edit: That didn't work because expanding the globs and passing them in to Karma has bad side effects. And the previous hacky version was brilliant reverse engineering, but was very hard to read / maintain, so I'd like to avoid it if possible. What if we just make it a requirement (via a comment) that Karma globs use extension-specific tokens? So if you want 'foo/bar/**/*.{js,ts}', you must specify 'foo/bar/**/*.js', 'foo/bar/**/*.ts'. It will greatly simplify the transform logic, and we can expand it in future to perform different transforms for different extensions.

build-system/tasks/runtime-test/runtime-test-base.js Outdated Show resolved Hide resolved
@rsimha
Copy link
Contributor Author

rsimha commented Mar 2, 2021

@jridgewell This needs one last look from you to decide how we should handle #32891 (comment).

rsimha and others added 6 commits March 2, 2021 17:15
karma-esbuild creates a new binary for **every** file, whereas karma-browserify collects all tests into a single binary. Our tests depend on this behavior, so we must collect all of the esbuild-files, create a single target to import all of them, and feed that file to esbuild.
@rsimha rsimha merged commit d5e1275 into ampproject:master Mar 2, 2021
@rsimha rsimha deleted the 2021-02-24-KarmaEsbuild branch March 2, 2021 23:56
@samouri
Copy link
Member

samouri commented Mar 3, 2021

I believe we lost cache invalidation in this PR, since it is only path based. I.e. it breaks watch mode for gulp dev server. Fix incoming as part of #32744

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants