Skip to content

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

Merged
cpcallen merged 3 commits into
RaspberryPiFoundation:developfrom
cpcallen:fix/7224/test-imports
Jul 7, 2023
Merged

cpcallen merged 3 commits into
RaspberryPiFoundation:developfrom
cpcallen:fix/7224/test-imports

Conversation

@cpcallen

@cpcallen cpcallen commented Jul 7, 2023

Copy link
Copy Markdown
Collaborator

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!

cpcallen added 3 commits July 7, 2023 10:15
This resolves the warnings generated during buildDeps by
closure-make-deps.

This commit does not attempt to address RaspberryPiFoundation#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 RaspberryPiFoundation#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 RaspberryPiFoundation: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

PR: fix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants