Skip to content

Migrate tests goog module#5440

Merged
alschmiedt merged 10 commits into
RaspberryPiFoundation:goog_modulefrom
alschmiedt:migrate_tests_goog_module
Sep 16, 2021
Merged

Migrate tests goog module#5440
alschmiedt merged 10 commits into
RaspberryPiFoundation:goog_modulefrom
alschmiedt:migrate_tests_goog_module

Conversation

@alschmiedt
Copy link
Copy Markdown
Contributor

@alschmiedt alschmiedt commented Sep 8, 2021

Proposed Changes

  • Adds a mocha_deps.js file that contains everything in core, generators, blocks, and mocha/tests.
  • Updates test_helpers.js to be a goog.module.
  • Updating test_helpers.js causes other tests to fail, so I updated a few as an example for how to fix the other ones.

We were uncertain if stubbing something like Blockly.utilx.genUid would work when we require it through `const utils = goog.require('Blockly.utils'). I tried the below and it seemed to work, but I'm not sure it is a comparable situation.

sinon.stub(utilsToolbox, 'convertFlyoutDefToJsonArray').returns('AHHHH');
console.log(Blockly.utils.toolbox.convertFlyoutDefToJsonArray({})); <-- Prints out AHHHH
console.log(utilsToolbox.convertFlyoutDefToJsonArray({}));<-- Prints out AHHHH

I also tried

In toolbox_test.js
sinon.stub(utilsToolbox, 'convertFlyoutDefToJsonArray').returns('AHHHH');
this.toolbox.aMethod();

In Toolbox.js
Toolbox.prototype.aMethod = function() {
  console.log(Blockly.utils.toolbox.convertFlyoutDefToJsonArray({})); <-- Prints out AHHH
};

Additional Information

I factored out testAWorkspace function into its own workspace_helpers.js file. It was being used in both workspace_test.js and workspace_svg_test.js so it made sense to me to create a workspace helper file same as we have procedure and toolbox helpers.

@alschmiedt alschmiedt requested a review from a team as a code owner September 8, 2021 00:18
@alschmiedt alschmiedt requested a review from maribethb September 8, 2021 00:18
@google-cla google-cla Bot added the cla: yes Used by Google's CLA checker. label Sep 8, 2021
@github-actions github-actions Bot added this to the 2021_q3_release milestone Sep 8, 2021
@cpcallen cpcallen self-requested a review September 8, 2021 00:23
@cpcallen
Copy link
Copy Markdown
Collaborator

cpcallen commented Sep 8, 2021

OK, this looks really promising as far as mocking goes.

As far as the issues you noted in chat:

Tests are still failing because I exported everything from test_helpers.js and they then need to be imported to the other tests where the helpers are used...I didn't know a way around this, but if there is one lmk and I can try to fix.

The only way around is through: all the test files that depend on the helpers need to become goog.modules too. Actually, that's not strictly true; we have a few choices:

  1. We could make all the test files into goog.modules.
  2. We could move the helpers that mock/stub things into a separate file, and only moduleify that file and the particular test files that depend on those particular functions.
  3. We could have test_helpers.js do goog.module.declareLegacyNamespace(), and then refer to the helpers by their fully-qualified names (suggestion: Blockly.test.helpers.*)

But I think that #2 and #3 are probably only procrastinating from doing #1.

Copy link
Copy Markdown
Collaborator

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

Looks good, but for some small stylistic issues.

I have commits in my local branch to apply the name changes (and convert a few more files to goog.module) that I can push to this PR if you like.

Comment thread scripts/gulpfiles/build_tasks.js
Comment thread scripts/gulpfiles/build_tasks.js Outdated
Comment thread tests/mocha/astnode_test.js
Comment thread tests/mocha/astnode_test.js Outdated
Comment thread tests/mocha/test_helpers.js Outdated
@alschmiedt
Copy link
Copy Markdown
Contributor Author

Happy to have you push whatever changes you have so far, and then I can finish up converting the rest of the files and applying any other comments you have on top of that.

@google-cla
Copy link
Copy Markdown

google-cla Bot commented Sep 8, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla Bot added cla: no Used by Google's CLA checker. and removed cla: yes Used by Google's CLA checker. labels Sep 8, 2021
@cpcallen
Copy link
Copy Markdown
Collaborator

cpcallen commented Sep 8, 2021

@googlebot I consent.

@google-cla google-cla Bot added cla: yes Used by Google's CLA checker. and removed cla: no Used by Google's CLA checker. labels Sep 8, 2021
Copy link
Copy Markdown
Collaborator

@cpcallen cpcallen left a comment

Choose a reason for hiding this comment

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

I think "Fix imports and workspace tests" might be a little under-descriptive of the changes actually contained in that commit: I was fairly confused about why you were moving most of the contents of workspace_test.js to workspace_helpers.js until I saw that testAWorkspace is now also being called from workspace_svg_test.js!

@cpcallen cpcallen assigned alschmiedt and unassigned cpcallen Sep 16, 2021
@alschmiedt alschmiedt force-pushed the migrate_tests_goog_module branch from 4f57f53 to f10532e Compare September 16, 2021 19:54
@alschmiedt alschmiedt assigned cpcallen and unassigned alschmiedt Sep 16, 2021
@alschmiedt alschmiedt merged commit 5b1586e into RaspberryPiFoundation:goog_module Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Used by Google's CLA checker. type: cleanup

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants