E2e tests overhaul #2024
E2e tests overhaul #2024
Conversation
| @@ -0,0 +1,639 @@ | |||
| import {paramCase} from 'change-case'; | |||
There was a problem hiding this comment.
I've added everything in a single file for easier reviewing, but eventually think this should be broken up.
| } | ||
|
|
||
| async function createWorkerProdServer({root, port}: ServerConfig) { | ||
| const mf = new Miniflare({ |
There was a problem hiding this comment.
Will replace with mini-oxygen in a follow up.
|
|
||
| expect(text).toBe('Hello world'); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Seeing this now in isolation - I like the colocation of component and test, but I don't like how it feels a little unclear (and magical?) how things are getting rendered and where it's coming from.
For example, it's not very clear that the default export above is getting rendered, or even how it's getting rendered.
Maybe this is an area where I wouldn't mind having to be more explicit, even if it means writing more code?
There was a problem hiding this comment.
I'm not sure I follow here, can you show an example of how you would prefer it to work?
The way I understand this, we render route X and we include the tests for route X in the same file... what's missing?
There was a problem hiding this comment.
I guess I'm just thinking about other test frameworks, where you call something like render(<Home/>), and was thinking that I like the explicitness of that.
Somewhat related: in the current situation, how would you change the props passed to <Home/> to test different code paths? Is that possible?
There was a problem hiding this comment.
I don't think that's possible since these are e2e tests and depends on what the browser sends 🤔 -- might be wrong though
| </> | ||
| ); | ||
| } | ||
| ` |
There was a problem hiding this comment.
It kind of stinks that these have to be written as strings, as you lose features such as linting, formatting, typechecking, etc. (especially as part of the CI, which is helpful for keeping external contributions cleaner too)
I guess you don't have to create them as strings, though, right? They can be files in the filesystem? These were just done for ease of use?
There was a problem hiding this comment.
Yeah these can be files, I also think there is probably a tagged template literal (similar to gql) that may solve some of the issues you identified. Either one exists of we could create something fairly easily.
There was a problem hiding this comment.
This is my main question as well... what's the benefit of having this as strings over having files? Do we have an example or use-case where this approach is clearly better? 🤔
Maybe we testing multiple routes so that we can colocate tests with the content of the files?
There was a problem hiding this comment.
Great job @cartogram! I think colocating tests in routes will make 90% of the tests easier to follow.
I have some questions about the whole meta framework:
- It looks like this initializes the
SandboxInstanceonce perdescribeand each environment, and the initialization is basically building or creating a dev server. I guess this means we need to be cautious when adding newdescribesbecause it can change the performance of the tests, and we should rather useitdirectly as much as possible. Is this correct? - Related to the point above, what would be the "recommended" way to organize tests now (maximizing perf, I guess)? At first glance, it's not clear to me because now we have much more flexibility than before. For example, do we have a project
test/cssthat has 1describeand includes tests for everything related to CSS, including Pure CSS and CSS Modules? Should those two be split in different describes or in different projects? - When do we write tests directly in test files instead of routes? When the tests are related to more than 1 single route? Any other use-case?
- I guess this is unknown yet but, any idea on how this compares with the previous approach in terms of performance? Before we had 2 big builds and 1 dev process (per project). I guess it's different now but perhaps the builds are faster?
- Is it possible to run these test projects locally for development? A common way to debug tests before was basically running the projects in dev but I'm not sure how to do that now that most of the files are inlined strings in test files.
- Do you think that it will be possible at some point to run each describe/environment in a different process?
| react: '^18.2.0', | ||
| 'react-dom': '^18.2.0', |
There was a problem hiding this comment.
I guess we could grab these from Hydrogen's package.json#peerDependencies?
|
Thanks for all the great questions @frandiox, some answers below:
Yes that's correct. There will be a balance between when we share a test suite (for performance) and we establish a new one (for isolation). I don't have an answer to this yet and will have to come out in practise, but important to remember that e2e is unlike unit testing in this way.
Yes, this is a great observation. I've considered a few options to make this easy (ie: new hydrogen config === new test suite), but haven't come up with a clear answer just yet.
I think it would be great if we did as much as possible within the route files since that is a nice API, and I am trying to come up with a way to support this across the board (inclusive of multiple routes). Again, not a clear answer but it is an improvement I would like to make to this whole framework design.
I do think this should possible and would be a big performance win. |
|
@cartogram Would it make sense to optionally pass the |
I think given this model, things like async config would be a different fixture personally. |
|
I think this can be closed 😆 |
|
Most likely, yes... 😅 |
This PR suggests a new approach to our e2e tests for the Hydrogen framework and showcases a few basic test-suites. This is a Proof of Concept, want feedback before devoting too much effort into it. While the broad characteristics of this overhaul are defined here and outlined below, there will be more methods and functions added as we move all of the e2e tests over.
Dynamic fixtures
Each app runs in an isolated sandbox that is created/removed before/after each test is run. This is different than our current set up, where all fixtures are checked into git and live in the packages/playground directory.
Our current setup has been problematic because:
packagesfolder should only contain actual packages from our monorepo and the additional files add unnecessary bloat. More files tends to lead to more confusion when trying to understand a single e2e test. This is solved with Dynamic Fixtures because these files will not exist in the repo at all unless a test is running or the{persist: true}config is given (this is for debugging only).Generating the fixtures dynamically also follows the pattern of other frameworks (next, remix, astro, etc...).
In-source testing for routes and components
An even further extension of the above point, many of our e2e tests are route or component specific and now we can define tests beneath the code they are testing. This is different than our current set up, where all the tests live in a single suite for each fixture and all the routes defined within that fixtures routes folder.
Our current setup has been problematic because:
Additionally, tests can even share variables, etc... between the source and test. This also follows a trend in remix's Route Module API, where a route-level component has multiple exports (
meta,linksetc... ) atestsexport falls right inline with this way of collocating everything in a top-level route file.Contained setup and teardown
We export our own
describefunction that defines each suite and takes care of the nitty setup/teardown of each test. This is different than our current set up, where we reference at least 4 different files for the building of code, starting servers, and other tasks required for each e2e test suit to run properly.The below files are not easy to reason about (this has come up in conversations with many on the team). The changes in this PR would remove all of them.
Our current setup has been problematic because:
The one draw back to this approach is that we are essentially "wrapping" vitest primitives and it may become unclear where that line is drawn. I might suggest we go full in on this approach and essentially re-export * from vitest inside of our test-framework so that a developer doesn't need to think about it. We could consider a "hydrogen/testing" package that sets this all up and a user test might look like:
Support for testing in multiple environments
Each suite runs in 3 environments: Node development using a vite dev server, Node production using our platform entry, and Worker production using our platform entry and worker runtime and we can opt-out of any of these at the suite level. This is different than our current set up, where we pass in a config to each test and use conditional blocks to return early if we don't want a test to run in a specific environment.
Our current setup has been problematic because:
The above conditional logic can be omitted if we are able to declaratively tell a test suite how and where to run.
Vitest
We have been moving away from Jest entirely in our repo, in favour of Vitest. Not much to add on this point because it is a hard requirement for vite 3 among other reasons (ESM support, etc).
cc @pepicrft @lemonmade @heimidal
Running these new tests locally
Checkout this PR and run
yarn test-e2e-new.