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

Resources extension: Consider running tests sequentially when lifecycle methods have resources #685

Closed
jbduncan opened this issue Nov 13, 2022 · 6 comments · Fixed by #690

Comments

@jbduncan
Copy link
Contributor

When tests and lifecycle methods share the same @Shared resource, they are forced to run sequentially, even if parallel tests have been enabled. This prevents methods from mutating the resource at the same time.

However, it is a common pattern in JUnit to save some data in a field from inside a lifecycle method. This is error-prone because if a resource is saved in a field by a @BeforeAll method, then the @Tests will run in parallel as they are not annotated with @Shared directly, and thus they are free to mutate the resource at the same time.

To prevent this, we should consider running all tests in a test class sequentially (including all @Nested classes) when the class has a lifecycle method with a @Shared parameter.

@nipafx
Copy link
Member

nipafx commented Nov 15, 2022

Storing something in a static field and then mutating it and then turning on parallel tests is really asking for problems - @Shared or not - so I'm not sure it's on us to try and fix that. (The same applies to non-static fields with a "per-class" test instance lifecycle, but I'm not sure those are even parallelized.

I'm inclined to "fix" this by adding a sentence or two to the documentation.

@Bukama
Copy link
Member

Bukama commented Nov 15, 2022

We could also considered ‚Isolated“

@nipafx
Copy link
Member

nipafx commented Nov 15, 2022

I don't follow. Can you elaborate?

@jbduncan
Copy link
Contributor Author

Storing something in a static field and then mutating it and then turning on parallel tests is really asking for problems...

Oh, I'd forgotten that @BeforeAll methods can only be static (unless they're in a class with a "per-class" lifecycle), so that's a good point indeed.

The following blurb already exists in the documentation, so I'm wondering if we even need to say any more?

Caution Be careful not to save resources in fields from any test method, including @BeforeAll and @BeforeEach methods, as this extension cannot guarantee that such resources are read or mutated sequentially.

@nipafx
Copy link
Member

nipafx commented Nov 15, 2022

I think that's ok. I'm editing the docs as we speak write - should I add this sentence?

Instead, use @Shared to reuse resources in multiple tests.

@jbduncan
Copy link
Contributor Author

I like it! It makes it clear what the user should be doing instead.

@nipafx nipafx linked a pull request Nov 15, 2022 that will close this issue
14 tasks
Michael1993 pushed a commit that referenced this issue Nov 17, 2022
* improve project description
* improve article titles, descriptions, and first paragraph (~> #681)
* sort navigation by title
* fix broken internal links
* use `====` for multi-line admonition boxes
* many small improvements (~> #685)

Closes: #681, #685
PR: #690
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants