8000 Use Blockly consistently in mocha tests · Issue #7224 · google/blockly · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use Blockly consistently in mocha tests #7224

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

Jump to bottom
Open
1 task done
maribethb opened this issue Jun 28, 2023 · 4 comments
Open
1 task done

Use Blockly consistently in mocha tests #7224

maribethb opened this issue Jun 28, 2023 · 4 comments
Assignees
@cpcallen
Labels
issue: bug Describes why the code or behaviour is wrong

Comments

@maribethb
Copy link
Contributor

Check for duplicates

  • I have searched for similar issues before opening a new one.

Description

In mocha test files, sometimes we use Blockly.foo and sometimes use import {foo} from '../build/.......'; foo

we shouldn't use both styles; that's just confusing.

Christopher says there are arguments to both. @cpcallen please elaborate in the comments :)

Without hearing those arguments, I would prefer using Blockly.foo because ideally the tests would use Blockly the same way users do and users don't import our individual files.

Reproduction steps

Stack trace

No response

Screenshots

No response

Browsers

No response
@maribethb maribethb added issue: bug Describes why the code or behaviour is wrong issue: triage Issues awaiting triage by a Blockly team member labels Jun 28, 2023
@cpcallen
Copy link
Contributor

Background

Tests can ideally be divided into roughly three categories:

  1. Unit tests, that test single functions/methods or perhaps single classes. It is common to stub/mock the dependencies of the code under test in order to isolate it, which helps make the tests less brittle and allows them to execute more quickly.
  2. Integration tests, in which multiple units of code (whole/multiple modules, perhaps) are tested to ensure that interactions between them are correct. Dependencies outside the integration being tested might still be stubbed/mocked, but this is often less necessary since the integration being tested might include most or all of the dependencies.
  3. System tests, in which an entire program/library is being tested, including all its dependencies.

In the case of system tests of a software library like Blockly, it behoves one to test against the API that one is shipping. (If it's also a UI library, then one should additionally also be testing the UI—in our case the DOM elements Blockly creates and manages.) So any kind of system tests we are doing ought to be against the exports of core/blockly.ts, i.e., (historically) Blockly.*.

In the case of unit tests, it is generally preferable to test against only the module containing the code being tested. This enables the unit tests for a single module to be run without having to compile/load any other modules—most test frameworks have the ability to specify exactly what tests should be run, making it convenient to quickly run the tests for a particular module while working on it, without having to repeatedly run the entire test suite. It also allows testing of interfaces that are not exposed by the public API (which for an app, rather than a library, is basically all of them).

(Aside: to facilitate keeping tests and tested code in sync, unit tests are often kept very close to the code being tested. The Go convention is to keep the tests for foobar.go in foobar_test.go in the same directory, with the latter having full namespace access to the former without any explicit, as if it were part of the same file. In scripting languages like Perl and Python, it is not unusual for libraries to include the unit tests in the same file as the code under test, executed when the file is run as a script rather than imported as a module.)

Blockly is in the curious situation of being a library and still maintaining most of its original goog.provides structure. This means that (barring places where we have made changes relatively recently):

  • We intentionally export most of our internal APIs.
  • The entire API for some module with ID foo.bar.baz was publicly available via the foo.bar.baz namespace whether we wanted it to be or not. At best we could politely ask that some parts be treated as @private.
  • Previous to the goog.module migration, there was no way to access this module's API other than via the foo.bar.baz namespace.

So we have a lot of unit tests that are written against Blockly.*, and it's not easy to see the difference between unit, integration, and system tests just by looking at what they import.

More recently, some test modules have been created/modified to directly import submodules. This is some cases necessary in order to test internal-only APIs (or use test-only APIs for mocking/stubbing), but in other cases (as in the code in #7200) this has happened because of some confusion about what the appropriate approach is.

Recommendations

Specifically on the topic of imports, I would recommend that we adopt the convention that unit tests directly import the module containing the code under test, while system tests continue to test against the top-level API (Blockly.*). This should 8000 eliminate most situations where a test has reason to both use the Blockly.* namespace and import an individual module. (Integration tests might go either way, though inertia will I suspect tend to favour the status quo.)

I also recommend we:

  • Learn more about modern best practices for testing TypeScript code and organising test code (especially: outside Google).
  • Consider reorganising our tests if that will help make their structure more familiar to external contributors.
  • Consider more clearly separating unit, integration, and system tests.
  • Consider moving unit tests closer to the code being tested (if there is a reasonably good precedent for doing so in TypeScript projects).
  • Learn (and document) how to use mocha to quickly run subsets of tests (e.g., unit tests for a single module).
  • Reorganise our test framework so that at least unit tests, and preferably as many other tests as possible (e.g. generators), can—but do not have to be—be run headlessly in node.js, rather than in a browser.
  • Reorganise our test framework so that existing unit- and non-unit mocha tests, generator tests, and the new browser tests, or any desired subset thereof, can be run via a single mocha invocation that opens (at most) a single browser instance.

@maribethb maribethb removed the issue: triage Issues awaiting triage by a Blockly team member label Jul 5, 2023
@maribethb maribethb mentioned this issue Jul 5, 2023
Open
@maribethb
Copy link
Contributor Author

I've moved the work in Christopher's comment above into a new issue: #7250

This issue covers only the removal of the import statements from current mocha tests, where possible. That is higher priority because some of them are affecting the build and emitting warnings. The other organizational work can wait and has therefore been separated out.

@cpcallen
Copy link
Contributor
cpcallen commented Jul 7, 2023

To be clear: the build warnings are not due to direct import statements; they are due to import statements that pull in paths that are not included amongst the inputs to closure-make-deps. These paths are incorrect and presumptively invalid, but evidently not sufficiently invalid that the import actually fails at runtime. How this is possible is worthy of further investigation, but fixing the invalid paths is trivial.

I think due consideration of #7250 remains a prerequisite to this issue, however, because our tests make extensive use of direct imports (the preponderance of which are valid and do not generate warnings):

Y
Filename uses Blockly.* uses import … '…build/src/core…'
astnode_test.js Y Y
block_json_test.js Y Y
block_test.js Y Y
comment_deserialization_test.js Y
comment_test.js Y Y
connection_checker_test.js Y Y
connection_db_test.js Y Y
connection_test.js Y
contextmenu_items_test.js Y
cursor_test.js Y Y
dropdowndiv_test.js Y
event_block_change_test.js Y
event_block_create_test.js Y Y
event_block_delete_test.js Y
event_block_drag_test.js Y
event_block_field_intermediate_change_test.js Y
event_block_move_test.js Y
event_bubble_open_test.js Y
event_click_test.js Y
event_comment_change_test.js Y
event_comment_create_test.js Y
event_comment_delete_test.js Y
event_comment_move_test.js Y
event_marker_move_test.js Y
event_selected_test.js Y
event_test.js Y Y
event_theme_change_test.js Y
event_toolbox_item_select_test.js Y
event_trashcan_open_test.js Y
event_var_create_test.js Y
event_var_delete_test.js Y
event_var_rename_test.js Y
event_viewport_test.js Y
extensions_test.js Y
field_angle_test.js Y Y
field_checkbox_test.js Y Y
field_colour_test.js Y Y
field_dropdown_test.js Y Y
field_image_test.js Y Y
field_label_serializable_test.js Y Y
field_label_test.js Y Y
field_multilineinput_test.js Y Y
field_number_test.js Y Y
field_registry_test.js Y Y
field_test.js Y Y
field_textinput_test.js Y Y
field_variable_test.js Y Y
flyout_test.js Y
generator_test.js Y Y
gesture_test.js Y Y
icon_test.js Y
input_test.js Y
insertion_marker_manager_test.js Y
insertion_marker_test.js Y
jso_deserialization_test.js Y Y
jso_serialization_test.js Y Y
json_test.js Y
keydown_test.js Y Y
metrics_test.js Y
mutator_test.js Y
names_test.js Y
procedure_map_test.js Y
registry_test.js Y
render_management_test.js Y
serializer_test.js Y Y
shortcut_registry_test.js Y
theme_test.js Y Y
toolbox_test.js Y
tooltip_test.js Y
touch_test.js Y
trashcan_test.js Y Y
utils_test.js Y
variable_map_test.js Y
variable_model_test.js Y
webdriver.js
widget_div_test.js Y
workspace_comment_test.js Y Y
workspace_svg_test.js Y Y
workspace_test.js Y
xml_test.js Y
zoom_controls_test.js Y Y
blocks/lists_test.js Y
blocks/logic_ternary_test.js Y Y
blocks/procedures_test.js Y Y
blocks/variables_test.js Y Y
test_helpers/block_definitions.js Y
test_helpers/code_generation.js Y
test_helpers/common.js Y
test_helpers/events.js Y
test_helpers/fields.js Y
test_helpers/icon_mocks.js Y
test_helpers/procedures.js Y Y
test_helpers/serialization.js Y
test_helpers/setup_teardown.js Y Y
test_helpers/toolbox_definitions.js Y
test_helpers/user_input.js Y Y
test_helpers/variables.js Y
test_helpers/warnings.js Y
test_helpers/workspace.js Y Y

@cpcallen
Copy link
Contributor
cpcallen commented Jul 7, 2023

OK, having investigated the invalid import paths issue:

  • In chat on the day of release I blamed fix: Utilize getIcon instead of getCommentIcon in tests #7200 for these warnings; this was partially true but alas I made exactly the same error in refactor(tests): Migrate mocha tests from goog.require to import #7196. Mea culpa.
  • The reason that the invalid import paths were not causing test failures was that it was preventing the files containing them from being successfully loaded at all. Because tests/mocha/index.html loads the test suites via <script> tags (via tests/bootstrap.js and the debug module loader in closure/goog/base.js), any errors in loading a test suite are reported only in the browser console, and do not otherwise cause test failures.
  • The result of this is that we were running only 2800 of (at least) 2975 tests. (I say "at least" because even after fixing these ones it's possible there are other loading failures that are not generating obvious console errors, so I can't be 100% confident all our test are being run.)
  • Predictably, one of the tests that was not being run is now failing:
    • Context Menu Items
      • Block Items
        • Comment
          X Creates comment if one did not exist 
          AssertionError: New block should not have a comment: expected undefined to equal null
at Context.<anonymous> (contextmenu_items_test.js:462:21)

I note that, should we want to convert the tests to TypeScript, we will need to pass tests/mocha through tsc (as we do for closure, core, blocks and generators already); this would involve removing …/build/src/ from any/all import paths in the files in tests/mocha/. We could opt to do so proactively, and doing so might reducing the likelihood of errors of this sort occurring in future (since the object files in build/src/tests/mocha/**/*.js would have same relative relative paths to build/src/core/**/*.js as the source files in tests/mocha/**/*.ts have to core/**/*.ts.)

cpcallen added a commit to cpcallen/blockly that referenced this issue Jul 7, 2023
@cpcallen
fix(tests): Fix invalid import paths in mocha tests
3dcf145 
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.
@cpcallen cpcallen mentioned this issue Jul 7, 2023
Merged
4 tasks
cpcallen added a commit that referenced this issue Jul 7, 2023
@cpcallen
fix(tests): Make test imports correct and more consistent (#7260)
aefb97d 
* fix(tests): Fix invalid import paths in mocha tests

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

  This commit does not attempt to address #7224 or otherwise
  rationalise the imports and usage thereof.

* fix(tests): Fix failing context menu item test

  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."

* refactor(tests): Clean up inconsistent usage of CommentIcon

  Tweak the test files touched by PR #7200 to be consistent with
  existing usage of Blockly.icons.CommentIcon instead of importing
  this separately.
@cpcallen cpcallen mentioned this issue Aug 17, 2023
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cpcallen cpcallen

Labels
issue: bug Describes why the code or behaviour is wrong
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@cpcallen @maribethb
0