Reducing QUnit reliance on ESM mutable facades - #34993
Open
vorobey wants to merge 12 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces QUnit runner reliance on generated “mutable facade” modules by moving stubbing/spying to explicit DEBUG setters and small test helpers, and by tightening the runner’s import-map/module-identity behavior to avoid multiple ESM instances.
Changes:
- Add a generic “seam” helper (
moduleSeam.js) and visibility-change-specific helpers to stub/spy ESM exports viaDEBUG_set_*functions. - Add
DEBUG_set_*hooks toexporter.jsandcommon/core/events/visibility_change.js, and migrate affected tests to use them. - Update the QUnit runner’s import map and auto-facade indexing strategy (new
NAMESPACE_FACADE_MODULES, reducedMUTABLE_MODULE_GROUPS) to reduce duplicate module instances and avoid broad filesystem scanning.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.viz.core/export.tests.js | Switch exporter stubbing from sinon.stub to seam-based stubbing for ESM compatibility. |
| packages/devextreme/testing/tests/DevExpress.viz.core/export.integration.tests.js | Same seam-based exporter stubbing migration for integration coverage. |
| packages/devextreme/testing/tests/DevExpress.viz.charts/charts.tests.js | Update tracker import style to match module’s default export shape. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets/resizable.tests.js | Use DEBUG_set_triggerResizeEvent instead of direct reassignment. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets/popup.tests.js | Replace direct spying with spyVisibilityEvent helper. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets/overlay.tests.js | Replace direct stubs/reassignments with visibility-change seam helpers and DEBUG setters. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets/gallery.tests.js | Replace direct spying with spyVisibilityEvent helper. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets/drawer.tests.js | Replace direct reassignment with DEBUG_set_triggerResizeEvent. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets.form/form.tests.js | Replace direct spying with spyVisibilityEvent helper. |
| packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/gridView.tests.js | Switch to DEBUG_set_triggerShownEvent for ESM-safe override (needs restore). |
| packages/devextreme/testing/runner/lib/static.ts | Adjust auto-facade generation call to match new tryBuildAutoMutableFacade signature. |
| packages/devextreme/testing/runner/lib/namespaceFacadeModules.ts | New explicit list of modules considered for namespace-facade registration. |
| packages/devextreme/testing/runner/lib/mutableModuleGroups.ts | Reduce forced mutable facades in favor of explicit DEBUG seams. |
| packages/devextreme/testing/runner/lib/importMap.ts | Map package-root entries to extensionless URLs to avoid duplicate module instances. |
| packages/devextreme/testing/runner/lib/autoMutableFacade.ts | Replace filesystem scanning with explicit NAMESPACE_FACADE_MODULES registration; simplify auto-facade lookup. |
| packages/devextreme/testing/helpers/visibilityChangeMock.js | New helper wrapping seam operations for visibility-change exports. |
| packages/devextreme/testing/helpers/moduleSeam.js | New generic seam helper for DEBUG-settable ESM exports (should validate setters). |
| packages/devextreme/js/exporter.js | Add DEBUG setter to allow test-time replacement of export. |
| packages/devextreme/js/common/core/events/visibility_change.js | Convert named exports to let + add DEBUG setters for test-time replacement. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
vorobey
changed the title
…com:DevExpress/DevExtreme into fix/qunit_mutable_facades
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The removed facade still backs the chart tracker mocks, so those mocks no longer replace the constructors used by chart code.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 150/150 changed files
- Comments generated: 1
- Review effort level: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.