fix: move test helpers from samples into core #5969

BeksOmega · 2022-03-01T22:46:34Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide

The details

Resolves

Proposed Changes

Moves tests helpers that were in samples into core. Creates a new namespace for helpers. Splits helpers into different files to help with organization.

Behavior Before Change

Core depended on test helpers in samples, which depended on core.

Behavior After Change

Test helpers still exist in samples (for now) but core no longer depends on them.

Reason for Changes

When these were samples, we had to keep releasing dev tools when doing changes to get CI to pass. This makes it easy to change them as we change things in core, if necessary.

Test Coverage

All unit tests pass.

Documentation

N/A

Additional Information

The only issue is that the field serialization tests no longer work, because they rely on the test blocks, which do still exist in samples. But I don't think that should be blocking.

Also: this PR should be ok to review commit-by-commit (assuming I didn't mess anything up when I reorganized a few commits) if that's easier.

gonfunko · 2022-03-02T16:32:10Z

tests/mocha/test_helpers/block_test_helpers.mocha.js

@@ -0,0 +1,240 @@
+/**


Throughout, was the conversion to goog modules necessary? It appears these were already ES modules in dev-tools, so moving backwards seems not ideal.

Honestly, not sure. Before it seems we were just accessing these via testHelpers which was a global thing. I could try to move back to that. Or is there a better way to access ES modules from inside closure modules?

I think you should be able to just do goog.declareModuleId() (per https://github.com/google/closure-compiler/wiki/Migrating-from-goog.modules-to-ES6-modules), and then still be able to goog.require it from Closure modules even though it's an ES module.

I tried, but for some reason it's "breaking" the tests. The suites are being evaluated, but the tests are not (based on console logging). You can check out my changes here.

As far as I can tell, what I did matches what you did in this commit, so I don't know what the problem could be. And I haven't been able to find anyone online having the same issue :/ Thoughts?

Looks like mochajs/mocha#2816, mocha is being run before the tests are loaded. Adding type="module" to the script tag at the bottom of index.html that calls mocha.run fixes it, and the tests pass!

Thank you so much Aaron! I should have been able to find that -.- But I was looking for a closure related problem. Turns out closure wasn't the issue at all!

tests/mocha/index.html

tests/mocha/block_test.js

tests/mocha/field_registry_test.js

tests/mocha/field_test.js

tests/mocha/test_helpers/toolbox_definitions.js

gonfunko

LGTM once tests are updated to run properly.

cpcallen · 2022-03-05T00:29:56Z

tests/mocha/astnode_test.js



 suite('ASTNode', function() {
+  console.log('1/a');


Was this debugging that got committed by accident? (Here and several places below).

cpcallen · 2022-03-05T00:32:44Z

tests/mocha/.eslintrc.json

    },
    "rules": {
        "no-unused-vars": ["off"],
        // Allow uncommented helper functions in tests.
        "require-jsdoc": ["off"],
-        "prefer-rest-params": ["off"]
+        "prefer-rest-params": ["off"],
+        "no-invalid-this": ["off"]


Probably much better to turn this off only where it is necessary. See e.g. core/utils/global.js.

cpcallen · 2022-03-05T00:43:17Z

tests/mocha/test_helpers/procedures.js

@@ -3,7 +3,7 @@
 * Copyright 2020 Google LLC
 * SPDX-License-Identifier: Apache-2.0
 */
-goog.module('Blockly.test.procedureHelpers');
+goog.declareModuleId('Blockly.test.helpers.procedures');


The tests can't be converted to ES Modules until the test harness is modified so they are not loaded directly from file:// URLs.

This change causes an endless stream of:

Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type of "". Strict MIME type checking is enforced for module scripts per HTML spec. block_definitions.js:1 Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type of "". Strict MIME type checking is enforced for module scripts per HTML spec. events.js:1

(etc.)

(The result of which is that the Mocha tests "pass" because none of them even get loaded, so there can be no failures. Zero failures === success, according to the test harness.)

cpcallen

Hmm. This one went in a little prematurely, I fear…

cpcallen · 2022-03-05T01:21:11Z

tests/mocha/test_helpers/common.js

+ * @template {TestCase} T
+ * @template {TestSuite} U


I note that this is causing additional warnings during build (specifically, while doing build:deps:

WARNING in /Users/cpcallen/src/blockly/tests/mocha/test_helpers/common.js at 38, 3: Bounded generic semantics are currently still in development WARNING in /Users/cpcallen/src/blockly/tests/mocha/test_helpers/common.js at 39, 3: Bounded generic semantics are currently still in development

(These two plus three more below).

I checked to see if there was some way to suppress these warnings, but I was not able to find one and filed google/closure-library#1156 to request such a feature.

BeksOmega requested a review from a team as a code owner March 1, 2022 22:46

BeksOmega requested a review from gonfunko March 1, 2022 22:46

github-actions bot assigned gonfunko Mar 1, 2022

BeksOmega added 18 commits March 2, 2022 15:45

fix: move core test helpers into new directory

99af064

fix: add test helpers to core and convert to goog modules

eff84af

fix: change tests to use local helpers

e276745

fix: change local tests to use chai asserts

fb57f18

fix: skip field tests in serializer b/c test blocks are unavailable

3a655c5

fix: rename some helper files

0ded768

fix: rename some helper modules

08cddc8

fix: split block helpers into code gen and serialization

6d47b07

fix: split block defs into new helper file

8a78f68

fix: split warning helpers into new file

e084e91

fix: split user input helpers into new file

ccfd888

fix: split event helpers into a new file

e1ad627

fix: split variable helper into new file

0766370

fix: move remaining test helpers to new setup-teardown file

b332bdf

fix: rename setup and teardown module

e2dd397

fix: cleanup from rebase

a72953c

fix: undo accidental rename

acb3e3b

fix: lint?

39db79b

BeksOmega force-pushed the refact/helpers-to-core branch from 042af0b to 39db79b Compare March 2, 2022 15:45

gonfunko reviewed Mar 2, 2022

View reviewed changes

BeksOmega added 3 commits March 2, 2022 23:12

fix: bad toolbox definitions namespace

9c13a36

fix: fixup warning helpers

0e39153

fix: remove inclusion of dev-tools in mocha tests

1f8eb8f

BeksOmega force-pushed the refact/helpers-to-core branch from 2c19b70 to 1f8eb8f Compare March 2, 2022 23:26

move to modules, but break mocha

220d7bb

gonfunko approved these changes Mar 4, 2022

View reviewed changes

fix: run mocha as a module

4dac25a

fix: lint

050956d

BeksOmega merged commit 2edd228 into google:develop Mar 4, 2022

release-please bot mentioned this pull request Mar 4, 2022

chore(develop): release blockly 8.0.0 #5888

Merged

cpcallen reviewed Mar 5, 2022

View reviewed changes

BeksOmega deleted the refact/helpers-to-core branch April 5, 2022 15:00

This was referenced Apr 21, 2022

chore(develop): release blockly 9.0.0 #6096

Closed

chore(develop): release blockly 9.0.0 #6100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: move test helpers from samples into core #5969

fix: move test helpers from samples into core #5969

BeksOmega commented Mar 1, 2022

gonfunko Mar 2, 2022

BeksOmega Mar 2, 2022

gonfunko Mar 2, 2022

BeksOmega Mar 3, 2022

gonfunko Mar 4, 2022

BeksOmega Mar 4, 2022

gonfunko left a comment

cpcallen Mar 5, 2022

cpcallen Mar 5, 2022

cpcallen Mar 5, 2022 •

edited

Loading

cpcallen Mar 5, 2022

cpcallen left a comment

cpcallen Mar 5, 2022

fix: move test helpers from samples into core #5969

fix: move test helpers from samples into core #5969

Conversation

BeksOmega commented Mar 1, 2022

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

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

gonfunko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcallen Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcallen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpcallen Mar 5, 2022 •

edited

Loading