Repository navigation
CI and automated tests - #19
Merged
Merged
Conversation
npm run lint has never been able to run: it invokes eslint but no config exists anywhere in the repo, so it exits with "couldn't find a configuration file". Nothing called it from CI, so it went unnoticed. Adds an eslintrc extending react-app, which was already a devDependency and is evidently what was intended. Scope is src and .storybook; demos are separate npm projects with their own configs and their own eslint-plugin-import, which collides with the root's - their coverage belongs with a per-demo build job instead. Cleared what it found: dead imports across the stories and the memory-debug wrapper. The four react-hooks/exhaustive-deps warnings are suppressed at each site rather than fixed - those effects run once per chart lifecycle by design, and the readme documents that initChart must not change, so adding the dependencies would tear down and re-create the surface. Storybook's !!raw-loader! imports are required for docs source display, so import/no-webpack-loader-syntax is off for .storybook only. Runs with --max-warnings 0. Without it eslint exits 0 on warnings, so the CI step would have passed while reporting problems - which is the failure mode this replaces.
Each demo is its own npm project, so nothing in CI compiled them. Four of the six failed to build during the 2.0 work - a Builder namespace removed in v6, two demos still on scichart 3.x, and a dev script that could not resolve its own dependency - and every one was found by hand. A matrix job that installs and builds each demo catches that class. The job installs and builds the library first: the feature demos and react16 resolve scichart from the root node_modules, and the others consume lib/ as a file: dependency, so it has to exist before their install runs. fail-fast is off so one broken demo does not mask the rest. electron-forge exposes no compile-only command, so packaging is that demo's build. Verified by running npm ci followed by the build for all six locally, rather than the npm install used while developing. That difference mattered: react18/basic's lockfile pinned @types/react 18.2.20, a version whose ReactNode definition makes JSX.Element unassignable, so the demo failed to compile. It surfaced now because the debug wrapper's props moved from PropsWithChildren<any> to a typed object, which is the first time its children were actually checked. The @types/react floor therefore moves to ^18.3.0 wherever React 18 types are declared, rather than only unpinning one lockfile entry - three demos asked for ^18.2.15 and one for ^18, so nothing stopped the bad version returning. The root moves with them: the feature demos and react16 compile the library from source, so the root's react types and theirs end up in the same program, and bumping only the demos put two copies of csstype in play and broke CSSProperties assignability across the tree boundary.
Three majors, and mostly a restructure rather than a version bump: addon-essentials, addon-interactions and blocks were all folded into core in 9, so those packages are gone and the docs blocks now come from @storybook/addon-docs/blocks. main.ts is ESM, the deprecated docs.autodocs option is dropped, and story types come from the framework package rather than the renderer. Three things the upgrade command did not handle: - webpack-merge was imported as a default export. That only worked through CommonJS interop; once main.ts became real ESM it resolved to undefined and the build died with "merge is not a function". It is a named export. - Since v8 the webpack5 builder ships no compiler at all, so nothing stripped TypeScript and every story failed to parse. Added the babel compiler addon rather than swc, so the existing .babelrc.json and its browser targets still apply. - preset-react ran in classic mode, so JSX compiled to React.createElement while no story imports React - Storybook 7 injected the automatic runtime itself. Now set explicitly, matching tsconfig's jsx: react-jsx. Also dropped @storybook/blocks and @storybook/testing-library, both left behind by the upgrade and no longer imported, and removed the addon-mcp the migration added unasked. Verified: all 13 stories render with no console errors, including 3D, pie, the group loader and the nested overview, with wasm still served from staticDirs.
The repo had no test runner at all. @storybook/test-runner renders each story in Chromium via Playwright and fails if it throws, which is the cheapest check that the components still mount against the engine - they are WebAssembly and WebGL, so nothing short of a real browser exercises them. @storybook/addon-vitest was the first choice and cannot be used here: it requires a Vite-based Storybook, and this project is react-webpack5. Migrating would also break every !!raw-loader! import the stories rely on for their source display. No .storybook/test-runner config file is present, and one cannot be added yet. The runner loads it through Jest, and Storybook 10.6 reads it with module.register(), which Jest rejects outright - the failure is identical for a .ts or a .js config. That costs the console-error assertions the hooks would have given; the default behaviour, failing on a story that throws, still holds. Verified by breaking a story on purpose: 10 of 13 tests failed with the injected error, and all 13 pass once reverted.
SciChartSurface.useWasmFromCDN and useWasmLocal are deprecated in favour of loadWasmFromCDN and loadWasmLocal, because the use* prefix trips eslint's react-hooks/rules-of-hooks - which makes them exactly the wrong names to show in a React wrapper's examples. All four references were in comments, so nothing executed a deprecated call.
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.
This adds proper CI and automated tests to scichart-react (SCJS-2747).
What changes:
npm run test:dockerruns the same tests locally.useWasmFromCDN.CONTRIBUTING.mdexplains the local checks, how to update the reference screenshots, and how to release. The note in the publish workflow about who can release is corrected.Note: the screenshot check fails on the first run because there are no reference images yet. They'll be taken from that run and committed to this PR.