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

fix(tests): Make test imports correct and more consistent #7260

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

cpcallen
Copy link
Contributor

@cpcallen cpcallen commented Jul 7, 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

Addresses the cause of #7224 being filed, though without making more than a token effort to address the issue as written.

Proposed Changes

  • Fix invalid import paths in mocha tests. This resolves the warnings generated during buildDeps by
    closure-make-deps.

  • Fix failing context menu item test:

    • The test appears to have been wrong: Block.prototype.getIcon is typed as
      getIcon(/* ... */): T | undefined
      and documented as "@returns The icon with the given type if it exists on the block, undefined otherwise."
  • Clean up inconsistent usage of CommentIcon in the test files touched by PR fix: Utilize getIcon instead of getCommentIcon in tests #7200 to be consistent with existing usage of Blockly.icons.CommentIcon instead of importing it separately.

Behaviour Before Change

  • Scary (but apparently not sufficiently scary) warnings generated by closure-make-deps during buildDeps.
  • Only 2800 mocha tests were being run, due to errors loading certain test files that were only being reported in the browser console (and one of the non-runners would have failed).

Behaviour After Change

  • No deps warnings.
  • 2976 mocha tests are run and none fail.

Reason for Changes

Better to run tests than not; better to pass than fail.

Test Coverage

Restored.

Additional Information

Still can't be sure that all mocha tests are actually being run!

This resolves the warnings generated during buildDeps by
closure-make-deps.

This commit does not attempt to address google#7224 or otherwise
rationalise the imports and usage thereof.
Test appears to have been wrong: Block.prototype.getIcon is typed
as

    getIcon<T extends IIcon>(/* ... */): T | undefined

and documented as "@returns The icon with the given type if it
exists on the block, undefined otherwise."
Tweak the test files touched by PR google#7200 to be consistent with
existing usage of Blockly.icons.CommentIcon instead of importing
this separately.
@cpcallen cpcallen requested a review from a team as a code owner July 7, 2023 11:02
@cpcallen cpcallen requested a review from BeksOmega July 7, 2023 11:02
@github-actions github-actions bot added the PR: fix Fixes a bug label Jul 7, 2023
@cpcallen cpcallen merged commit aefb97d into google:develop Jul 7, 2023
@cpcallen cpcallen deleted the fix/7224/test-imports branch July 7, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants