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

refactor(tests): Use import instead of goog.bootstrap to load Blockly in mocha tests #7406

Merged
merged 7 commits into from
Aug 18, 2023

Conversation

cpcallen
Copy link
Contributor

@cpcallen cpcallen commented Aug 17, 2023

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Part of #6858.

Proposed Changes

  • Have the buildShims task clean up after itself.

    We need to create a build/package.json file to allow node.js to load build/src/core/blockly.js and the other chunk entry points as ES modules (it otherwise forcibly assumes .js means CJS even if one is trying to import the file rather than require it, unless package.json says {"type": "module"}), but this interferes with scripts/migration/js2ts doing a require('build/deps.js'), since deps.js is not an ES module.

  • Temporarily add missing imports of closure/goog/goog.js to modules in tests/mocha/ (see de9499b for rationale).

  • Migrate tests/mocha/index.html to import the loader shims instead of using bootstrap.js.

    • It now uses import to load Blockly, the library blocks and the JavaScript generator via the shims create in PR refactor(tests): Introduce loading shims, use in playgrounds #7380. Because the mocha tests have always been run in uncompressed mode it could instead directly import the chunk entry point s (e.g. build/src/core/blockly.js), but using the shims allows us to optionally run the tests in compressed mode as well. At the moment they do not all pass, because several tests directly import individual modules from core/, which is not compatible with blockly_compressed.js, but Use Blockly consistently in mocha tests #7224 is tracking work that would resolve this problem.
    • It also now uses import to load the individual test files that were originally goog.require()ed and more recently loaded via bootstrap.js.
    • It uses loadScript (from tests/scripts/load.mjs) to load various additional scripts including en.js.
  • Remove code from build_tasks.js that generates tests/deps.mocha.js, since it was only needed by the bootstrap code now deleted from tests/mocha/index.html.

  • Remove goog.declareModuleId calls (and added imports) from all tests/mocha/**/*.js.

  • Remove some dead code from tests/mocha/test_helpers/serialization.js.

Behaviour Before Change

Mocha tests run and 2994/2994 (100%) tests pass in uncompressed mode.

Behaviour After Change

Mocha tests run and 2994/2994 (100%) tests pass in uncompressed mode.

Moreover, tests can optionally be run in compressed mode, with 2174/2994 (97%) passing.

Reason for Changes

Remove dependency on bootstrap.js so it and the Closure debug module loader can be deleted.

Test Coverage

Unchanged.

Additional Info

Due to the large number of files touched it may be easier to review commit-by-commit.

We need to create a build/package.json file to allow node.js to
load build/src/core/blockly.js and the other chunk entry points
as ES modules (it forcibly assumes .js means CJS even if one is
trying to import, unless package.json says {"type": "module"}),
but this interferes with scripts/migration/js2ts doing a
require('build/deps.js'), which is _not_ an ES module.

Specific error message was:

/Users/cpcallen/src/blockly/scripts/migration/js2ts:56
require(path.resolve(__dirname, '../../build/deps.js'));
^

Error [ERR_REQUIRE_ESM]: require() of ES Module
/Users/cpcallen/src/blockly/build/deps.js from /Users/cpcallen/src/blockly/scripts/migration/js2ts
not supported.
deps.js is treated as an ES module file as it is a .js file whose
nearest parent package.json contains "type": "module" which
declares all .js files in that package scope as ES modules.
Instead rename deps.js to end in .cjs, change the requiring code
to use dynamic import() which is available in all CommonJS
modules, or change "type": "module" to "type": "commonjs" in
/Users/cpcallen/src/blockly/build/package.json to treat all .js
files as CommonJS (using .mjs for all ES modules instead).

    at Object.<anonymous> (/Users/cpcallen/src/blockly/scripts/migration/js2ts:56:1) {
  code: 'ERR_REQUIRE_ESM'
}
These modules were depending on being loaded via the
debug module loader, which cannot be used without first loading
base.js as a script, and thereby defining goog.declareModuleId
as a side effect—but if they are to be loaded via direct import
statements then they need to actually import their own
dependencies.

This is a temporary measure as soon the goog.declareMouleId
calls can themselves be deleted.
This file was only needed by tests/mocha/index.html's use of
the debug module loader (via bootstrap.js), which has now been
removed.
These were only needed because these modules were previously
being loaded by goog.require and/or goog.bootstrap.
We are fully committed to proper modules now.
@rachel-fenichel
Copy link
Collaborator

This is a lot, but commit-by-commit it all makes sense.

If you have not already done so, please make sure that everything still works/tests pass after running npm run clean && npm run build.

@cpcallen cpcallen merged commit 6f20ac2 into google:develop Aug 18, 2023
12 checks passed
@cpcallen cpcallen deleted the refactor/6858/mocha branch August 18, 2023 17:06
cpcallen added a commit to cpcallen/blockly that referenced this pull request Aug 20, 2023
This was deleted in PR google#7406 as it was mainly being used to
filter core/ vs. test/mocha/ deps into separate deps files -
but it turns out also to be used for filtering error
messages too.  Oops.
cpcallen added a commit that referenced this pull request Aug 30, 2023
* fix(build): Restore erroneously-deleted filter function

  This was deleted in PR #7406 as it was mainly being used to
  filter core/ vs. test/mocha/ deps into separate deps files -
  but it turns out also to be used for filtering error
  messages too.  Oops.

* refactor(tests): Migrate advanced compilation test to ES Modules

* refactor(build): Migrate main.js to TypeScript

  This turns out to be pretty straight forward, even if it would
  cause crashing if one actually tried to import this module
  instead of just feeding it to Closure Compiler.

* chore(build): Remove goog.declareModuleId calls

  Replace goog.declareModuleId calls with a comment recording the
  former module ID for posterity (or at least until we decide
  how to reformat the renamings file.

* chore(tests): Delete closure/goog/*

  For the moment we still need something to serve as base.js for
  the benefit of closure-make-deps, so we keep a vestigial
  base.js around, containing only the @provideGoog declaration.

* refactor(build): Remove vestigial base.js

  By changing slightly the command line arguments to
  closure-make-deps and closure-calculate-chunks the need to have
  any base.js is eliminated.

* chore: Typo fix for PR #7415
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.

3 participants