New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add e2e tests for CTB restarts from file changes #19801
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
FYI there is also this historical PR with a bunch of good tests #18065 |
I'm going ahead and opening this PR as-is with the I'm very open to better ideas, but trying to watch the logs for the restart / started also feels overly complicated and potentially flakey as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, well done for cracking this, super hard stuff. Just some minor comments to discuss
// TODO: each test should have a beforeAll that does this, maybe combine all the setup into one util to simplify it | ||
// to keep other suites that don't modify files from needing to reset files, clean up after ourselves at the end | ||
test.afterAll(async () => { | ||
await resetFiles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you doing it afterAll but also in beforeEach
? Maybe it should be in afterEach
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a tricky question here.
In this file, the correct thing would be only the beforeEach
(since each test makes changes) and then other suites would ensure on their own it was clean with a beforeAll/beforeEach as needed. But doing the filesystem reset is slow and only a couple suites make file changes (ctb and uploading), and I didn't want to update every other test file to do that.
So I put the "bad" code (trying to clean up for other tests in afterAll is bad practice because you're not guaranteed it will run) separate even though it means we run it one extra time, because ultimately I want to remove it altogether once we have a faster method to detect/reset the files that would be ok to add universally.
V nice well done |
not quite, one of the runs failed 😞 I think I have another solution though that I'll try to run through the CI several times and see if it still happens |
What does it do?
I'm not super happy with how hacky it is, but it seems stable enough from the time I've been working with it. The most important thing I would like to find a solution for is the
delay()
to wait for the server to realize files have changed and it begins its restart process. However, without access to the output of the server I can't think of anything better. Perhaps a custom winston logger could pipe it somewhere that we could watch that would be realtime?Why is it needed?
We were limited by being unable to test the CTB because of the server restarts, filesystem changes, etc.
Roadmap
After this PR, I am planning the following changes:
v5/main
todevelop
(and then work from there on further changes)How to test it?
Running the full test suite with multiple browsers (that is, re-using the same running instance of strapi across multiple test runs of the same suite, and re-using the test-apps between runs ie without using
--setup
) should work every time. And continuing to run them many times in a row, they should always succeed without flakiness.Related issue(s)/PR(s)
DX-1166