Remove puppeteer test delays #2110
Comments
|
@raineorshine a couple of questions on this.
I think (4) is most easily solvable by me just doing distinct settings for each and then letting you review the changes and tell me which can be combined (if any) since that puts the onus on me to find them first. |
|
I think this answers a couple questions: All transitions should be disabled during the tests. That is the simplest and most effective approach. There is never a case where an automated test would cover the animation itself, just the beginning and end states. I originally wrote this issue before the PandaCSS conversion (which is still in progress). The As for shared transitions, I agree let's keep them separate for now (except the ones that have already been combined). We can combine later in a refactor, but I would say that’s a separate scope from this work. |
|
One clarification needed: The CSSTransition classes are React components that take an integer timeout value in milliseconds. The PandaCSS tokens are string values and include the units (e.g., Example PandaCSS: durations: {
highlightPulseDuration: {
value: {
base: '500ms',
_test: '0s',
},
},
sideMenuSlideDuration: {
value: {
base: '300',
_test: '0',
},
},
}, |
|
Good question. I think we want to use the panda config as the source of truth. Otherwise we would have, for example, If the panda config is the source of truth, then unfortunately we have to assume the durations are specified as a string in seconds or milliseconds. We could have a convention of only specifying durations in milliseconds, but that would require a custom eslint plugin to enforce in an Open Source context where many different developers with different levels of familiarity are expected to contribute to the project. As ugly as it is to define durations as a string and then parse them out again to use them in const toMilliseconds = (str: string) =>
str.endsWith('ms') ? parseInt(str, 10) :
str.endsWith('s') ? parseInt(str, 10) * 1000 :
parseInt(str, 10)Then you can use any of the duration tokens in JS: toMilliseconds(token('durations.highlightPulseDuration')) |
|
The pattern of zero'ing out a duration for the tests in the panda config is so common that I would recommend a small helper function: const duration = (str: string) => ({
value: {
base: str,
_test: '0s',
}
})Then the duration definitions will look like this: durations: {
highlightPulseDuration: duration('500ms'),
hoverPulseDuration: duration('300ms'),
...
}Given that these are only defined in a map within the panda config, we can go even further. I'll leave the definition up to you: durations: testableDurations({
highlightPulseDuration: '500ms',
hoverPulseDuration: '300ms',
...
}) |
|
I am running into two problems with this. Problem #1: The render thought tests are throwing an error when running the The comments in the Problem #2: The following error is cropping up in the
|
|
To clarify, is Problem 1 occurring in One possible explanation is that by the time Problem 2 looks like the spellchecker to be honest. I'm not sure why that is happening now, but maybe there is a way to disable it in the puppeteer config. |
|
No, problem 1 is not occurring in main but I've opted to keep the problematic duration changes for the end so I'll debug a bit more when I circle back. I did some digging on #2 and I do not believe there is even a way to disable it in Chromium. I have a StackOverflow question pending about it but my suggestion is to just not do anything until I hear back unless it starts killing PR checks. |
|
It looks like you discovered a slightly bigger problem: It appears that semantic tokens do not return the raw value like a token does but instead returns a variable: E.g., token('durations.noteColorTransitionDuration') === "var(--durations-note-color-transition-duration)"Tokens themselves do not allow for selectors like We may have to split out the My current, half-baked thought is having a file that:
|
|
Is there a possible solution where, when we are in the e2e tests, we do something like this? |
Hmmm, that is annoying. I was hoping Panda could be the source of truth, but css variables are not so usable in JS land.
This is a great idea, but |
If Panda cannot be the source of truth for durations, then they must be defined in JS. You can then import them into the Panda config. I think that's what you're suggesting in (1) but I could be wrong. Then |
My bad, I suggested this originally without testing |
FYI: This failed one of the CI runs on UPDATE: CI seems to be passing consistently now: 
|
Your solution makes sense; I'm not getting any bites on an answer so I think that's what we have to do for now. |
|
I submitted a PR with enough Since the tests run in parallel, the total time is basically the time of the longest test suite, so at this point there's not a huge amount of value in spending a bunch of time trying to shorten tests that are < 20 seconds unless people are frequently running individual tests. Regardless, the hamburger menu animation adds a significant amount of time to the tests overall since the longest running test suites have like ten individual tests. However, we could probably break those test suites down into multiple tests suites, let them run in parallel, and then get significant gains without any additional work. My proposal would be to:
|
|
That sounds like a good plan! Thanks for breaking it down like that. |
|
There are two sleeps remaining and I think we should save those for later. They are:
|
|
Perfect. I agree with your judgments! |
Disable all CSS transitions
on autofocus, thoughts, modals, and alertsduring tests.Having to wait for the animations slows down snapshot tests considerably.
The work has been started in Convert test animation durations to PandaCSS #2343. Take a look at how the PandaCSS _test condition is utilized to conditionally set the animation durations to 0s. Any remaining CSS transitions that slow down the tests (namely modals and alerts) need to be converted.
Once animations are disabled, remove the artificial delays that were added to the puppeteer tests due to the need to wait for the animations.
If a delay cannot be removed but can be reduced, reduce it as much as possible. Instead of
sleep, consider waiting for an element to become visible.Make sure there are no intermittent failures by running the tests multiple times.
The text was updated successfully, but these errors were encountered: