Migrate from ember-cli-mirage to msw and @mswjs/data
#10393
Conversation
Turbo87
added
A-frontend 🐹
C-internal 🔧
Turbo87
force-pushed
the
msw
branch
5 times, most recently
from
87a8963 to
400fa6c
Compare
|
Hmm, since // ember-cli-build.js
...
return require('@embroider/compat').compatBuild(app, Webpack, {
extraPublicTrees,
staticAddonTrees: true,
staticAddonTestSupportTrees: true,
staticModifiers: true,
packagerOptions: {
webpackConfig: {
resolve: {
fallback: {
// disables `crypto` import warning in `axe-core`
crypto: false,
// disables `timers` import warning in `@sinon/fake-timers`
timers: false,
},
},
externals: ({ request, context }, callback) => {
if (request == 'msw' && context.includes('@mswjs/data')) {
return callback(null, request, 'global');
}
callback();
},
},
},
});I'm not sure if this is more better than the patch approach though 😅 |
Turbo87
force-pushed
the
msw
branch
22 times, most recently
from
7b13aa9 to
331b2d5
Compare
Turbo87
changed the title
ember-cli-mirage to msw and @mswjs/dataember-cli-mirage to msw and @mswjs/data
There was a problem hiding this comment.
I haven't reviewed all the changes yet, just leaving a quick feedback after skimming through them. However, I love the move to msw, which seems much better than Mirage. Since it supports both browser and Node.js, it makes our E2E code assertions more natural :D
There was a problem hiding this comment.
OK, so the caveat here is that my attitude towards reviewing test code is generally quite a bit laxer than it is towards production code, and my attitude towards meta-test code is laxer still. (Meta-meta-test code, however, overflows and I start taking it super seriously.)
Or, put another way, I didn't scrutinise every single line in a test case converted from the Mirage store API to the MSW database API. The tests succeeding is what I'm more interested in, honestly.
Overall, though, this feels like it would have been worth it even if ember-cli-mirage wasn't on the way out. Going from 160 globalThis references to 40 alone feels good. The MSW API is more intuitive to interact with. There's just, overall, less magic involved.
And, most importantly, all but one of the tests still pass for me locally. (I made a note on the one that didn't in the inline comments.)
In summary: let's do it. 👍
| - `packages/crates-io-msw` - A mock backend used for testing | ||
| - `node_modules/` - npm dependencies - (ignored in `.gitignore`) |
There was a problem hiding this comment.
I can't suggest the actual right place in GitHub's UI, but this is less wrong:
| - `packages/crates-io-msw` - A mock backend used for testing | |
| - `node_modules/` - npm dependencies - (ignored in `.gitignore`) | |
| - `node_modules/` - npm dependencies - (ignored in `.gitignore`) | |
| - `packages/crates-io-msw` - A mock backend used for testing |
| // Prevent `@mswjs/data` from bundling the `msw` package. | ||
| // | ||
| // `@crates-io/msw` is importing the ESM build of the `msw` package, but | ||
| // `@mswjs/data` is trying to import the CJS build instead. This is causing | ||
| // a conflict within webpack. Since we don't need the functionality within | ||
| // `@mswjs/data` that requires the `msw` package, we can safely ignore this | ||
| // import. |
There was a problem hiding this comment.
I don't think I want to know how long it took you to figure this out.
There was a problem hiding this comment.
yeah..... it took a while 😂
| ...handlers, | ||
| http.get('/assets/*', passthrough), | ||
| http.all(/.*\/percy\/.*/, passthrough), | ||
| http.get('https://:avatars.githubusercontent.com/u/:id', passthrough), |
There was a problem hiding this comment.
Is the colon before the hostname correct here? I can't find an obvious reference to what this does in the MSW docs, but I also haven't read them exhaustively.
Secondary question: should we pass this through? It feels like this potentially makes tests sensitive to live avatar changes on GitHub.
There was a problem hiding this comment.
GitHub uses multiple avatar subdomains and the colon is acting as a placeholder/wildcard.
without the pass-through we would be seeing images that don't load. we could potentially redirect it to an avatar image with the repo, but the pass-through is essentially just replicating the current behavior.
There was a problem hiding this comment.
Makes sense, thanks!
we could potentially redirect it to an avatar image with the repo
I think we probably should do that, but not in this PR. 🙂
| @@ -151,7 +151,9 @@ test.describe('/settings/tokens/new', { tag: '@routes' }, () => { | |||
| await expect(page.locator('[data-test-api-token="1"] [data-test-expired-at]')).toHaveCount(0); | |||
| }); | |||
|
|
|||
| test('token expiry', async ({ page }) => { | |||
| test('token expiry', async ({ page, msw }) => { | |||
There was a problem hiding this comment.
This test may have become locale-dependent in some cases:
1) [chromium] › e2e/routes/settings/tokens/new.spec.ts:154:7 › /settings/tokens/new › token expiry @routes
Error: Timed out 5000ms waiting for expect(locator).toHaveText(expected)
Locator: locator('[data-test-expiry-description]')
- Expected string - 1
+ Received string + 3
- The token will expire on 18 February 2018
+
+ The token will expire on February 18, 2018
+
Call log:
- expect.toHaveText with timeout 5000ms
- waiting for locator('[data-test-expiry-description]')
9 × locator resolved to <span data-test-expiry-description="" class="_expiry-description_6v2mdb">↵ The token will expire on February 18, …</span>
- unexpected value "
The token will expire on February 18, 2018
"
162 | let expectedDate = expiryDate.toLocaleDateString(undefined, { dateStyle: 'long' });
163 | let expectedDescription = `The token will expire on ${expectedDate}`;
> 164 | await expect(page.locator('[data-test-expiry-description]')).toHaveText(expectedDescription);
| ^
165 |
166 | await page.fill('[data-test-name]', 'token-name');
167 | await page.locator('[data-test-expiry]').selectOption('none');
at /home/adam/trees/rust-foundation/crates.io/e2e/routes/settings/tokens/new.spec.ts:164:66
1 failed
[chromium] › e2e/routes/settings/tokens/new.spec.ts:154:7 › /settings/tokens/new › token expiry @routes
There was a problem hiding this comment.
huh, interesting. I'll see if I can replicate that.
There was a problem hiding this comment.
I don't know which locale the headless browser will have chosen, honestly, but locale reports LC_TIME="en_AU.UTF-8".
| const row1 = page.locator('[data-test-row="0"]'); | ||
| await expect(row1.locator('[data-test-crate-name]')).toHaveText(baz.name); | ||
| await expect(row1.locator('[data-test-description]')).toHaveText(baz.description); | ||
| const row0 = page.locator('[data-test-row="1"]'); | ||
| await expect(row0.locator('[data-test-crate-name]')).toHaveText(bar.name); | ||
| await expect(row0.locator('[data-test-description]')).toHaveText(bar.description); |
There was a problem hiding this comment.
Is there any reason why the variable names row0 and row1 are being swapped here?
There was a problem hiding this comment.
the rev deps are ordered by downloads and the factory defaults for the downloads have slightly changed because the data package is 1-indexed instead of 0-indexed like mirage 😅
There was a problem hiding this comment.
Hmm, I was just confused by the variable names and selectors. row0 mapped to [data-test-row="1"], and row1 mapped to [data-test-row="0"]. Alternatively, perhaps we could change the variable names to rowBar and rowBaz, which would be less confusing?
There was a problem hiding this comment.
ah I see what you mean. I'll fix the misalignment.
As discussed in #10357,
ember-cli-mirageisn't particularly well maintained anymore and blocking a couple of dependency updates.This PR migrates us from using
ember-cli-miragefor the mock API in the frontend test suites towards usingmswand@mswjs/data.I've used a pnpm workspace to add a subpackage at
packages/crates-io-msw, that is roughly equivalent to the previous top-levelmiragefolder. This package contains:mswrequest handlers to simulate our crates.io API (exported ashandlers)@mswjs/datadatabase with all of the models that support the above request handlers (exported asdb)mirage/fixtures/and aloadFixtures()functionThe package also contains its own dedicated test suite. The test suite is essentially a vitest port of the previous
tests/mirage/tests, but running in Node.js to simplify their development.The
@crates-io/mswpackage is integrated into the QUnit test suite by asetupMsw()function (similar to the previoussetupMirage()). This function sets up the mock service worker and database for each test and ensures proper cleanup of both after the test has finished.The package is also integrated into the Playwright test suite (at
e2e/). Here, it is using amswfixture, that can be used in tests and sets upplaywright-mswwhich transforms themswrequest handlers into Playwright request handlers. Themswfixture has three properties,workeris controlling the HTTP mocking,dbis the@mswjs/datadatabase mentioned above, andauthenticateAs()is roughly equivalent to the similarly-named function from before. Since the mocking is now running on the Playwright side instead of inside the browser the test code could be simplified significantly.I'm aware that the diff is big, and I'm happy to split it up into more manageable chunks if it helps. I definitely recommend reviewing in "ignore whitespace changes" mode. I've tried to group the commits thematically so that the can be reviewed in smaller chunks too. Let me know if/how I can help in getting this reviewed! :)
Closes #10357
Related:
ember-cli-miragewith directmiragejsusage #10357