Skip to content
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

Disable YJS connecting status in tests #2111

Closed
raineorshine opened this issue Jul 6, 2024 · 7 comments · Fixed by #2453
Closed

Disable YJS connecting status in tests #2111

raineorshine opened this issue Jul 6, 2024 · 7 comments · Fixed by #2453
Labels
test Testing and project configuration

Comments

@raineorshine
Copy link
Contributor

raineorshine commented Jul 6, 2024

When there are no thoughts, the UI is kept blank or says "Connecting..." until the YJS websockets connects or time out after 5 sec. This slows down the tests considerably.

Disable this loading screen when in test mode and go straight to the thoughts.

Remove the sleep in test setup and make sure the tests pass.

@raineorshine raineorshine added the test Testing and project configuration label Jul 6, 2024
@raineorshine raineorshine added this to the ✅ Test Engineering milestone Jul 6, 2024
@raineorshine raineorshine added the performance Improve performance label Jul 21, 2024
@raineorshine raineorshine removed the performance Improve performance label Aug 12, 2024
@raineorshine raineorshine self-assigned this Aug 16, 2024
@raineorshine raineorshine removed their assignment Sep 1, 2024
@mstrofbass
Copy link
Contributor

@raineorshine this request appears to require changing the logic below to hide the LoadingEllipsis component if we're in the e2e tests:

status === 'preconnecting' ? null : status === 'connecting' || (isLoading && status !== 'offline') ? (
// (except when offline, otherwise the loading ellipsis will be shown indefinitely in the rare case where the tutorial has been closed but there are no thoughts)
<LoadingEllipsis delay={1500} text={status === 'connecting' ? 'Connecting' : 'Loading'} center />
) : // tutorial no children

Is there a mechanism to determine whether we're in the e2e tests within that component?

@raineorshine
Copy link
Contributor Author

You can detect if the app is running in puppeteer with navigator.webdriver.

I'm not sure if just hiding the LoadingEllipsis will be enough, or if you'll have to mess with the https://github.com/cybersemics/em/blob/main/src/stores/offlineStatusStore.ts which has some timeouts built in.

@mstrofbass
Copy link
Contributor

Okay, are you suggesting that a possible way to do this is to set the offline timeout to zero? Then this would filter down and effectively hide LoadingEllipsis?

I like that but I am not sufficiently familiar with the downstream impacts of setting the offline timeout to zero to fully understand the impact thereof.

That being said, I have tested both options and they seem to work fine. For both solutions I have gotten a difficult to reproduce intermittent failure with one or more of the snapshot and am continuing to debug to figure out what the cause is.

I'll note that the sleep in the test setup has 500ms specifically to wait for the hamburger animation to complete. I have gone ahead and removed it while debugging to see if I encounter any issues and it does not seem to cause any additional problems on my machine but I'm not sure if that would be the case in CI/CD.

Can you clarify whether I should leave the 500ms sleep portion for the hamburger animation or should I go ahead and remove the entire call to sleep?

@mstrofbass
Copy link
Contributor

FYI, it looks like the failure is in the url.ts test and this is the snapshot diff and error:

Error: Expected image to match or be a close match to snapshot but was 0.13916666666666666% different from snapshot (668 differing pixels).

single-line-1-diff

@raineorshine
Copy link
Contributor Author

raineorshine commented Oct 6, 2024

Okay, are you suggesting that a possible way to do this is to set the offline timeout to zero? Then this would filter down and effectively hide LoadingEllipsis?

I like that but I am not sufficiently familiar with the downstream impacts of setting the offline timeout to zero to fully understand the impact thereof.

Theoretically that would work (just like changing the animation durations to 0) and has the least impact on the code, because the start state, end state, and wait logic can stay the same. I say let's do it and we can circle back if we notice any issues.

I'll note that the sleep in the test setup has 500ms specifically to wait for the hamburger animation to complete. I have gone ahead and removed it while debugging to see if I encounter any issues and it does not seem to cause any additional problems on my machine but I'm not sure if that would be the case in CI/CD.

Can you clarify whether I should leave the 500ms sleep portion for the hamburger animation or should I go ahead and remove the entire call to sleep?

Disabling the hamburger animation would fall under the scope of #2110. The goal is to remove as many sleeps as possible from the tests, switch to polling, or, as a last resort, reduce the sleep duration as much as possible.

FYI, it looks like the failure is in the url.ts test and this is the snapshot diff and error:

Error: Expected image to match or be a close match to snapshot but was 0.13916666666666666% different from snapshot (668 differing pixels).

I reproduced the url test on my machine and took a video to get a closer look. It actually looks like there is a bug that causes a slight layout shift of the last thought. You can see it at about 00:04:

Untitled.mov

It may be intermittent because there is a race condition between the snapshot and the layout shift.

It's a bit annoying that it interferes with our work on the tests in this milestone. Let me know if you'd like to try to fix the issue (billed as additional work at your hourly rate), or just skip the test and create an issue to come back to it.

@mstrofbass
Copy link
Contributor

Disabling the hamburger animation would fall under the scope of #2110. The goal is to remove as many sleeps as possible from the tests, switch to polling, or, as a last resort, reduce the sleep duration as much as possible.

I'll remove the websocket sleep portion under this issue and remove the hamburger animation sleep portion under #2110

It's a bit annoying that it interferes with our work on the tests in this milestone. Let me know if you'd like to try to fix the issue (billed as additional work at your hourly rate), or just skip the test and create an issue to come back to it.

Let's create a new issue and come back to it at the end.

@raineorshine
Copy link
Contributor Author

I'll remove the websocket sleep portion under this issue and remove the hamburger animation sleep portion under #2110

Sounds good!

Let's create a new issue and come back to it at the end.

#2452

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Testing and project configuration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants