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

Enable setting global default setup options #904

Closed
CreativeTechGuy opened this issue Apr 5, 2022 · 10 comments
Closed

Enable setting global default setup options #904

CreativeTechGuy opened this issue Apr 5, 2022 · 10 comments
Labels
enhancement New feature or request

Comments

@CreativeTechGuy
Copy link
Contributor

Problem description

This is related to #833.

If we are using fake timers in all tests, that means that we now must specify { delay: null } in every test when we call userEvent.setup.

Suggested solution

In the spirit of dom-testing-library's configure method it'd be nice to have a top-level function which can set the default options for userEvent.setup so we don't have to pass them in every time.

Additionally, having an option to bypass the fake timers for this library rather than having to turn off the delay entirely would be even better! (I'd argue this should be default since it's a very difficult bug to track down when upgrading this library and suddenly having tests timeout.)

Additional context

No response

@CreativeTechGuy CreativeTechGuy added enhancement New feature or request needs assessment This needs to be looked at by a team member labels Apr 5, 2022
@CreativeTechGuy CreativeTechGuy mentioned this issue Apr 5, 2022
3 tasks
@CreativeTechGuy
Copy link
Contributor Author

I took a shot at implementing what I'm imagining here and when using it to upgrade my projects, it was a much better experience. I'd love to hear what you think!

@ph-fritsche
Copy link
Member

First of all thanks for taking the time to contribute to this library ❤️

Let's try to keep issues separate. That makes it easier to discuss them and reference them later. (No harm in filing multiple short issues at once.)

I fear a global config does more harm than good.
We want to motivate people to use userEvent.setup after the DOM is ready because it ensures that our workarounds are applied (see #714 ) and it gives a clear entry point where manually reenacting a test workflow would start which makes it a lot easier to help people with their tests.
If you want to apply a config across all your tests, wrapping render and userEvent.setup in a setup function and applying your config there is probably the best way to solve this.

Regarding the fake timers: A PR resolving #585 would be welcome.
Note that Jest isn't the only test environment this library is used in. I think the user should supply the callback.

type Config = {
  // ...
  advanceTimers: (delay: number) => Promise<void>
}

@ph-fritsche ph-fritsche removed the needs assessment This needs to be looked at by a team member label Apr 5, 2022
@CreativeTechGuy
Copy link
Contributor Author

Understood! I'll separate the fake timers into another PR and we can discuss more over there.

My thought with the global config was a few things:

  • It aligns with the rest of the @testing-library ecosystem which has the same approach.
  • It'll be easier to motivate people to use the new .setup API since it'll be easier to use. (less duplication and no need for everyone to write another abstraction) It doesn't replace it or take away any of the benefits of it, it just avoids the duplication given that almost every setup call with have the same arguments by default.
  • I saw the setup function approach and it feels like a solution to a problem that could be solved at the source. The fact that wrappers are needed makes me think that we need a way to set global defaults. The pattern of global setup is very common in testing frameworks and it seems to fit nicely.

Can you elaborate on how it does more harm? People would still need to use .setup before firing any event, the only difference is that they would likely need less arguments (or none) when calling it.

@CreativeTechGuy
Copy link
Contributor Author

The PRs have been split. Thanks for taking a look. Hopefully we can come up with a solution that is best for the users! 😃

@ph-fritsche
Copy link
Member

ph-fritsche commented Apr 6, 2022

Can you elaborate on how it does more harm? People would still need to use .setup before firing any event, the only difference is that they would likely need less arguments (or none) when calling it.

Changes to properties on imported modules have the potential to obfuscate what is happening in a test.
If you jump into some test and there is a function call at the start of it, you can easily follow the call(s) in your IDE and see what is supposed to happen - and if you move the test code somewhere else, it behaves the same.
But if there is a function call which behaves differently due to some global config which can be mutated anywhere,
it might not be obvious why the test code behaves like it does - and if you move the test code somewhere else, it might behave differently.
Kent describes the problem for before loops in https://kentcdodds.com/blog/avoid-nesting-when-youre-testing very well.

@CreativeTechGuy
Copy link
Contributor Author

So just to be clear, you disagree with the standards set by the @testing-library ecosystem (see the configure method) and would like to go against them despite being part of the same organization? I totally get your point and I think it's valid. But consistency is also very important and it's arguably more confusing if each library does something a different way and requires different workarounds to be written by the end user. Especially when from a user's perspective, all of the libraries are from the same source.

@ph-fritsche
Copy link
Member

ph-fritsche commented Apr 6, 2022

If exposing the configure method was the best decision at that time, I don't know.
I do know that there are undocumented properties on it that are treated as internals.
The main reason for this function to exist is the plugin nature of the framework libraries. The user only imports e.g. @testing-library/react and this automatically alters the underlying @testing-library/dom which is not directly imported by the vast majority of our users.

I've never used it in any tests and never seen any bug reports or questions that were resolved by pointing out a configuration issue with the documented properties.
Introducing the .setup() function resolved some issues that caused confusion and resulted in bug reports / questions that were not easily to identify as either. I fear that a global config might result in more of such issues and there aren't many people around offering support on this.

@CreativeTechGuy
Copy link
Contributor Author

Okay. Just to confirm, .setup() only needs to be called after JSDOM is setup. So if it was called once and reused between all tests that would be totally fine. Or if it was called outside of a test case, that'd be fine. Just so long as it's called at some point explicitly so that listeners can be setup on the DOM, right?

So instead of needing a global config method, we can instead do:

myUserEvent.js

import realUserEvent from "@testing-library/user-event";
export const userEvent = realUserEvent.setup({ /* my global config here */ });

myTestFile.test.js

import { userEvent } from "./myUserEvent"
test("something", async () => {
    userEvent.click(button);
});

If so, this seems like a much easier option for migration than what is in the docs (returning it bundled with the render method).

@ph-fritsche
Copy link
Member

Correct, for the workarounds it doesn't matter where it is called. But in accordance with "Avoid Nesting When You're Testing" we recommend to .setup() in each test.
This provides a clean input device state.

import userEvent from '@testing-library/user-event'
const user = userEvent.setup()

test('type something', async () => {
  render(<input/>)
  // If you move this after the following test,
  // it won't type because Ctrl+Char has no `keypress`.
  await user.type('something')
  // ...
})

test('click with control', async () => {
  render(<button/>)
  await user.keyboard('{Control>}')
  await user.click(screen.getByRole('button'))
  // ... Control is still pressed
})

@CreativeTechGuy
Copy link
Contributor Author

Got it. I guess this will never be accepted so I'll close this issue and the related PR. Thanks for discussing with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants