Add Mocha testing to examples/inventory-app #26168
Conversation
Remove jest, puppeteer, and all test-related configuration to simplify the example. Webpack dev server is preserved for running the example. - Delete jest.config.cjs and jest-puppeteer.config.cjs - Delete tests/main.test.ts - Remove test scripts and 11 test devDependencies from package.json - Clean up tsconfig.json to remove test type references 🤖 Generated with [Nori](https://nori.ai) Co-Authored-By: Nori <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive Mocha-based testing to the inventory-app example, enabling it to be fully ESM-compatible by removing the dependency on Jest (which requires CommonJS builds).
Key changes:
- Adds new test suite with Fluid container creation tests, React component rendering tests, and document state handling tests
- Migrates from Jest to Mocha for ESM compatibility
- Adds testing infrastructure (Mocha, @testing-library/react, global-jsdom)
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Locks new testing dependencies (Mocha, @testing-library/react, global-jsdom, @types/mocha, @types/node) |
tsconfig.json |
Excludes test directory from main build to prevent test code from being included in production output |
src/test/tsconfig.json |
New TypeScript configuration for test files extending the standard test configuration |
src/test/inventoryApp.test.tsx |
New comprehensive test suite covering treeDataObject creation, TreeViewComponent rendering, and MainView integration |
package.json |
Adds test dependencies, test scripts, and build:test script for compiling tests |
.mocharc.cjs |
Standard Mocha configuration with exit flag (consistent with other examples) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
brrichards
force-pushed
the
inventory-mocha-tests
branch
2 times, most recently
from
348e317 to
da4d65e
Compare
brrichards
force-pushed
the
inventory-mocha-tests
branch
from
da4d65e to
9cc837b
Compare
examples/data-objects/inventory-app/src/test/inventoryApp.test.tsx
Outdated
Show resolved
Hide resolved
| import { MainView } from "../view/inventoryList.js"; | ||
|
|
||
| describe("inventoryApp", () => { | ||
| it("treeDataObject", async () => { |
There was a problem hiding this comment.
generally, tests are named after what they are testing, and test something in the package they are in. As treeDataObject is a utility from another package (which itself has tests for it), that name doesn't make a lot of sense here.
I'm not sure what the intent of this test is. It looks like its testing treeConfiguration's ability to use used treeDataObject and an actual fluid service ( TinyliciousClient ).
That however doesn't really test anything specific to this packager/example: that just adding another test case for treeDataObject and TinyliciousClient.
If this test was doing some actual collaboration and testing that the MainView from this package handled that correctly, then it would make sense to be here (to show how apps can test collab). This test however is only using one thing from the package its testing, the treeConfiguration, which is a declarative description of the schema and doesn't really need much testing (other than perhaps tests that its compatible with previous versions of the app so it can open old documents: that's what a real app would want to test for its schema).
If there were significant custom code in this schema (like methods on particular nodes) that needed testing, unit tests for those would make sense, but they would have no need to full a full service implementation as you could instead just test them on some unhydrated nodes, or an independent view (like the ui tests below) if you needed an associated view for them to work.
A good general heuristic is to start with tests that map 1:1 with the code being tested, then if necessary add some additional tests for how those components integrate together.
tree's src/test/README.md has some good guidance: while that specific to the tree package, most of it is good general guidance. We don't need to be as rigorous as tree in our testing since this is an example and not an exported library with users, but the general approach still applies.
For this specific test, if you want to test data object stuff, the only data object thing in this package to test is InventoryListFactory. Currently this test replicated something similar but not equivalent to that, then tests it, which doesn't really test the code in the package. A test named InventoryListFactory which actually uses InventoryListFactory would make more sense.
The logic you have below editing the tree and asserting you get the correct results could be rewritten as a test of the schema in examples/data-objects/inventory-app/src/schema.ts.
You can basically test the same thing by just saving the new Inventory node into a local variable and editing that without involving a client or even a view.
| describe(`StrictMode: ${reactStrictMode}`, () => { | ||
| const builder = new SchemaFactory("tree-react-api"); | ||
|
|
||
| class Item extends builder.object("Item", {}) {} |
There was a problem hiding this comment.
These are supposed to be tests for the inventory-app. Defineing a new schema this app does not use doesn't seem relevant to testing this app.
Nor does defining a new react component view (View below). I don't think the TreeViewComponent test accomplishes anything other than testing that this test setup and some of its dependencies work, which is something already handled by the tests of those dependencies (such as the tests in our react package).
I don't think we should be doing UI testing for anything that is not a view implemented in this app (meaning the MainView test you have below is what we want, and if we wanted more, we would test other views it implements)
examples/data-objects/inventory-app/src/test/inventoryApp.test.tsx
Outdated
Show resolved
Hide resolved
| assert.equal(rendered.baseElement.textContent, "View"); | ||
| }); | ||
|
|
||
| it("renders MainView with inventory data", () => { |
There was a problem hiding this comment.
This app contains 3 implementations of MainView, demoing different ways to implement it. Looping over [MainView, InventoryViewMonolithic, InventoryViewWithHook] so you can generate a test for each would be good here.
| assert.match(rendered.baseElement.textContent ?? "", /Inventory:/); | ||
| assert.match(rendered.baseElement.textContent ?? "", /bolt/); | ||
| assert.match(rendered.baseElement.textContent ?? "", /nut/); | ||
| }); |
There was a problem hiding this comment.
After validating that the view rendered cirrectly like this, it would be good to test invalidation.
For example check that tr tree contains bolt: 10 or something like that, then set it to some other number and check that it updated. THat would be a muhc more useful test than the editing case you have above, as this would test that the edit updates the view, which it the thing that it likley to have problems.
|
Looking at the tests in the CI run for this https://dev.azure.com/fluidframework/public/_build/results?buildId=371822&view=ms.vss-test-web.build-test-results-tab and searching for "renders MainView with inventory data" we can see that your new test was included (twice, for strict and non-strict) as expected, so it seems to be working. I think the configuration is good and working, but I left some suggestions of the tests themselves. |
2 participants
Adds Mocha testing to inventory-app package to be pure ESM (no CommonJS build)
New test suite (src/test/inventoryApp.test.tsx):
Package updates:
The previous test suite had one test with a commented-out line and was entirely skipped. Jest also required CommonJS builds since it doesn't support ESM natively. This change enables the example to be ESM-only while having meaningful test coverage by utilizing React and jsdom.