Repository navigation
Use Blockly consistently in mocha tests #7224
Description
Activity
Background
Tests can ideally be divided into roughly three categories:
- 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.
- 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.
- 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.goinfoobar_test.goin 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.providesstructure. 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.baznamespace 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.modulemigration, there was no way to access this module's API other than via thefoo.bar.baznamespace.
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 eliminate most situations where a test has reason to both use theBlockly.*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.
I've moved the work in Christopher's comment above into a new issue: #7250
This issue covers only the removal of the
importstatements 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.Reacted by Christopher AllenTo be clear: the build warnings are not due to direct
importstatements; they are due toimportstatements that pull in paths that are not included amongst the inputs toclosure-make-deps. These paths are incorrect and presumptively invalid, but evidently not sufficiently invalid that theimportactually 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):
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 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 OK, having investigated the invalid
importpaths 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.requiretoimport#7196. Mea culpa. - The reason that the invalid
importpaths were not causing test failures was that it was preventing the files containing them from being successfully loaded at all. Becausetests/mocha/index.htmlloads the test suites via<script>tags (viatests/bootstrap.jsand the debug module loader inclosure/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 existAssertionError: New block should not have a comment: expected undefined to equal null at Context.<anonymous> (contextmenu_items_test.js:462:21)
- Comment
- Block Items
- Context Menu Items
I note that, should we want to convert the tests to TypeScript, we will need to pass
tests/mochathroughtsc(as we do forclosure,core,blocksandgeneratorsalready); this would involve removing…/build/src/from any/all import paths in the files intests/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 inbuild/src/tests/mocha/**/*.jswould have same relative relative paths tobuild/src/core/**/*.jsas the source files intests/mocha/**/*.tshave tocore/**/*.ts.)- 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
Check for duplicates
Description
In mocha test files, sometimes we use
Blockly.fooand sometimes useimport {foo} from '../build/.......'; foowe 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.foobecause 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