Skip to content

fix(tests): Fix & reenable the advanced compilation test #5781

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.

Sign up for GitHub

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

Merged
rachel-fenichel merged 2 commits into from
Dec 3, 2021
Merged

fix(tests): Fix & reenable the advanced compilation test #5781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8370647
fix(test): Fix advanced compilation test
cpcallen Dec 3, 2021
a42fae7
chore(build): Reenable advanced compilation test
cpcallen Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/blockly.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,7 +742,7 @@ exports.zelos = zelos;
// Blockly.Msg module - so make sure it is, but only if there is not
// yet a Blockly global variable.
if (!('Blockly' in globalThis)) {
Copy link
Collaborator

@rachel-fenichel rachel-fenichel Dec 3, 2021

Choose a reason for hiding this comment

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

Is there a case where there was already a Blockly global variable and we need to be able to refer to it by Blockly.Msg instead of by importing it by name?

Copy link
Collaborator Author

@cpcallen cpcallen Dec 6, 2021

Choose a reason for hiding this comment

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

I'm not sure I understand the question, so I will say a few things in the hopes that one of them answers it:

  • At this point, barring an oversight on our part, none of the code in core/, blocks/ or generators/ ever assumes the existence of a Blockly global variable.
  • Aside from (optionally named) goog.require statements in tests, etc., the only code in that should create a Blockly global is this hack that tries to ensure that gloablThis.Blockly.Msg (and only that) exists, and the call to declareLegacyNamespace at the top of this file that will create a global Blockly variable when this file is loaded in uncompiled mode or (with help from the UMD wrapper) if blockly_compressed.js is imported with a <script> tag into the browser.
  • Most "outside" code should be importing Blockly via either an ES import statement or, if using Closure modules/compiler, a goog.require (e.g., Blockly Games and the advanced compilation test (ACT)).
    • I'm actually slightly unclear about importing Blockly using import works outside of node, as I wasn't aware that browsers have a way of loading UMD modules in response to ES import statements, but I would generally expect that no globals would be created as a result unless the import was being done as a named import at the top (script) level, in which case the importer is probably going to name the import Blockly.
    • For BG and the ACT, AVANCED_OPTIMIZATIONS results in everything (including Blockly) being renamed, so there a bunch of random 1- and 2-character variable names that are meaningless without a sourcemap.
  • In either case, unless the importer has gone out of their way to ensure that the Blockly global variable exists and points at the export object of this (blockly.js / Blockly) module, trying to load translations via <script> will fail. This means that the following excerpt from tests/compile/index.html: 
      <script src="main_compressed.js"></script>
  <script src="../../msg/js/en.js"></script>
    would throw when en.js tried to do Blockly.Msg["ADD_COMMENT"] = "Add Comment" because Blockly.Msg was undefined (until I fixed the issue in the first commit (8370647) of this PR).

globalThis['Blockly'] = {Msg};
globalThis['Blockly'] = {'Msg': Msg};
}

// Temporary hack to copy accessor properties from exports to the
Expand Down
12 changes: 2 additions & 10 deletions tests/compile/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,8 @@ goog.require('Blockly');
goog.require('Blockly.geras.Renderer');
goog.require('Blockly.VerticalFlyout');
// Blocks
goog.require('Blockly.Constants.Logic');
goog.require('Blockly.Constants.Loops');
goog.require('Blockly.Constants.Math');
goog.require('Blockly.Constants.TestBlocks');
goog.require('Blockly.Constants.Text');
goog.require('Blockly.Constants.Lists');
goog.require('Blockly.Constants.Colour');
goog.require('Blockly.Constants.Variables');
goog.require('Blockly.Constants.VariablesDynamic');
goog.require('Blockly.blocks.procedures');
goog.require('Blockly.blocks.all');
goog.require('Blockly.blocks.testBlocks');

Main.init = function() {
Blockly.inject('blocklyDiv', {
Expand Down
2 changes: 1 addition & 1 deletion tests/compile/test_blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/
'use strict';

goog.provide('Blockly.Constants.TestBlocks');
goog.provide('Blockly.blocks.testBlocks');

goog.require('Blockly');

Expand Down
4 changes: 2 additions & 2 deletions tests/run_all_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ run_test_command "package" "npm run package"
# Run Node tests.
run_test_command "node" "./node_modules/.bin/mocha tests/node --config tests/node/.mocharc.js"

# # Attempt advanced compilation of a Blockly app.
# run_test_command "advanced_compile" "npm run test:compile:advanced"
# Attempt advanced compilation of a Blockly app.
run_test_command "advanced_compile" "npm run test:compile:advanced"


# End of tests.
Expand Down