Skip to content

Conversation

@nichtsam
Copy link
Contributor

@nichtsam nichtsam commented Feb 18, 2025

Related: #931

Flakiness reasons:

  • Some tests check for pending states, but states might resolve before they get verified.
  • faker.internet.username() can generate usernames that exceed the length limit defined in SignupFormSchema.
  • Some tests interact with components that require JS before it's fully loaded (e.g., Radix Checkbox).

Test Plan

Checklist

  • Tests updated
  • Docs updated

Screenshots

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks good to me! Thanks!

@nichtsam
Copy link
Contributor Author

nichtsam commented Feb 18, 2025

That looks good to me! Thanks!

Hey! I’ve been working on this partly to showcase the issues, which is why I added the reliableAction logic. I’m not sure if it’s the best solution, but if you think it works well, I can mark it as ready for review.

I don’t think I’ve fixed all the flakiness yet, but I believe these changes have reduced it quite a bit—at least on my device 😅.

@kentcdodds
Copy link
Member

I don't really want to go with the reliable action approach. I would like to dig a little deeper.

@nichtsam nichtsam marked this pull request as ready for review February 25, 2025 12:54
@nichtsam
Copy link
Contributor Author

I don't really want to go with the reliable action approach. I would like to dig a little deeper.

I dug a bit deeper and found that one issue was JavaScript not being loaded. Since the page is SSR-ed, the visibility check always passes, and Playwright proceeds with the click. However, the Radix checkbox requires JavaScript to function, so the checked state isn’t updated, resulting in ineffective clicks.

@nichtsam nichtsam requested a review from kentcdodds February 25, 2025 13:01
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are good improvements. Thank you a lot!

@kentcdodds kentcdodds merged commit dfbc31c into epicweb-dev:main Feb 25, 2025
5 checks passed
@nichtsam nichtsam deleted the pr/playwright-test branch March 2, 2025 16:12
@andrecasal
Copy link
Contributor

I've identified two situations where tests fail:

  • A page.goto() does a full page reload, which requires hydration. If we interact with any form components before hydration occurs, the interaction fails.
  • When we click a link to load another page, individual JS files are loaded for each component the client hasn't had a change to request yet. This doesn't require hydration, but it does require the loaded JS to run before we interact with the components. However, there's an exception: if the component required for the linked page has been loaded at some point before clicking the link, the client has already cached that JS and it just needs to run—which is considerable faster in comparisong to network requests, but not necessarily a guarantee that the test will pass.

Manually tracking page.goto() and which components have been loaded in previous pages isn't a reliable or transparent technique.

So my question is: wouldn't progressively enhanced components, i.e. components that work correctly before JS has loaded/with JS disabled, and also respect whatever state the component had upon hydration/JS run, be a better approach? Am I missing anything? What's your take on this @kentcdodds?

These test failures could just as easily occur in slow networks too.

@kentcdodds
Copy link
Member

Thanks for bringing this up.

You can't always rely on progressive enhancement, and on top of that, most users are going to have JavaScript actually running. So it is better to simulate the actual environment that users are going to experience rather than rely on progressive enhancement, which most users will not be relying on.

@andrecasal
Copy link
Contributor

Thanks for the reply, Kent, appreciate you weighing in.

To clarify my thinking: the argument for progressive enhancement isn't about users disabling JavaScript (which is rare), but rather accommodating for real-world scenarios where JS loads and executes progressively due to slow networks. This is pretty common, like when people are in spotty coverage areas (e.g., remote from cell towers, in Faraday cage-like buildings, metro tunnels, or basements), leading to staggered loading of scripts and components.

With progressively enhanced components, users could start interacting with the page right away without frustrating issues, such as form inputs resetting upon hydration, which happens with Conform-controlled fields, for example. This approach not only improves the overall user experience on slower connections but could also make e2e tests less flaky by reducing reliance on perfect timing for JS execution, better mirroring those variable network conditions.

That being said, "progressively enhanced" might not be the most accurate term here. We're not aiming to deliver a basic, functional experience without JS and then layer on advanced features once it becomes available. Instead, the goal is to ensure that when JavaScript finally loads or hydrates, it synchronizes seamlessly with the current state of the underlying HTML, respecting any interactions the user may have already made, like entering data into forms.

I thought it was worth the effort to create such UI components.

@kentcdodds
Copy link
Member

I think it really depends on what your priorities are and your user base. The vast majority of users are using your site with JavaScript enabled. If the progressive enhancement case matters for you, then having specific tests for that makes sense. I don't think that it's significant enough for this project for us to do that by default though, so I think we'll keep things the way they are.

@nichtsam
Copy link
Contributor Author

I agree with Kent.

Running with and without JavaScript are two different environments. It’s not a testing oversight, but a matter of scope.
Since the project’s Guiding Principles specify to cover only the most common use cases, which are with JavaScript enabled, omitting progressive enhancement tests seems reasonable to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants