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

Do not run 'pollutedTests' task as a dependency of 'test' #1193

Closed
abelsromero opened this issue Apr 26, 2023 · 3 comments · Fixed by #1194
Closed

Do not run 'pollutedTests' task as a dependency of 'test' #1193

abelsromero opened this issue Apr 26, 2023 · 3 comments · Fixed by #1194
Milestone

Comments

@abelsromero
Copy link
Member

pollutedTest Gradle task is set as a dependency for test.
That's on it's own a bat pattern (https://melix.github.io/blog/2021/10/gradle-quickie-dependson.html) but means there's no way to run tests without it.

Imo, we should look for an alternative to run the test without resorting to this pattern. Plus, this is especially an issue when running from IDE, since the polluted test will run always when trying to run a test class or method independently.

@abelsromero
Copy link
Member Author

I've found https://junit-pioneer.org/, which has an extension to inject environment variables with an annotation.

I run a test on the junit5 PR branch and both of the following options work 👍

  1. Add annotation to test directly
@SetEnvironmentVariable(key = "GEM_PATH", value = "/some/path")
@SetEnvironmentVariable(key = "GEM_HOME", value = "/some/other/path")
public class WhenAnAsciidoctorClassIsInstantiatedInAnEnvironmentWithGemPath {
  1. Add pioneer annotations to our custom Polluted annotation.
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
// @Tag("polluted")
@SetEnvironmentVariable(key = "GEM_PATH", value = "/some/path")
@SetEnvironmentVariable(key = "GEM_HOME", value = "/some/other/path")
public @interface Polluted {
}

In both cases we can remove the 'polluted' task configuration from Gradle, so WhenAnAsciidoctorClassIsInstantiatedInAnEnvironmentWithGemPath runs as any other test and the IDE issues disappear. 👉 Less code and simpler build, I say it's a win, the only downside is we can't reuse it for v2.5.x, so as for me it will remain as it is.

Of the two options, I personally prefer the second, which adds some semantics to our discussions. We can refer to "polluted tests" and know exactly what it is. So I'd suggest proceeding with that as soon as JUnit5 is merged.

@robertpanzer
Copy link
Member

Let's go with the second option then.
I am somewhat curious how it should be possible to run a test with a different class path if this is an anti pattern?
(And I've thought of adding sth like build.dependsOn(pollutedTest) and it did not feel right to me, if I run ./gradlew test I want all tests to run.)

@abelsromero
Copy link
Member Author

Based on the article the anti-pattern is the use of dependsOn on its own, in our case there's no actual real dependency between test and pollutedTest, it's just convenient to get the later run when using the test task. But as an example of the bad consequence from that is that we cannot run them in parallel, when in fact we could.
One thing we do at work is having multiple Test tasks (integration, acceptance, ...) and reserve test for units only, then we just invoke the necessary ones ./gradlew accepanceTest test. In CI it means having a longer command, locally is the issue where you need to be aware of that. That's why -while annoying- I'd rather keep it as it is in v2.5.x, not being a daily projec I don't trust myself remembering to run which task 😅
I admit I don't understand why pollutedTest task does not catch results though 🤔

On the topic of a different classpath I think we'd need to define a Gradle configuration and use that for the other test task.

@abelsromero abelsromero added this to the 3.0.0 milestone May 1, 2023
abelsromero added a commit to abelsromero/asciidoctorj that referenced this issue May 1, 2023
* Add junit-pioneer to inject environment variables into tests. This allows
removing custom Gradle task pollutedTest and running them normally.
* Refactor WhenAnAsciidoctorClassIsInstantiatedInAnEnvironmentWithGemPath
as a parametrized tests.
* Add missing cases for Asciidoctor Factory in
WhenAnAsciidoctorClassIsInstantiatedInAnEnvironmentWithGemPath.

Fixes asciidoctor#1193
abelsromero added a commit to abelsromero/asciidoctorj that referenced this issue May 1, 2023
* Add junit-pioneer to inject environment variables into tests. This allows
removing custom Gradle task pollutedTest and running them normally.
* Refactor WhenAnAsciidoctorClassIsInstantiatedInAnEnvironmentWithGemPath
as a parametrized tests.
* Add missing cases for Asciidoctor Factory in
WhenAnAsciidoctorClassIsInstantiatedInAnEnvironmentWithGemPath.

Fixes asciidoctor#1193
abelsromero added a commit to abelsromero/asciidoctorj that referenced this issue May 2, 2023
* Add junit-pioneer to inject environment variables into tests. This allows
removing custom Gradle task pollutedTest and running them normally.
* Refactor WhenAnAsciidoctorClassIsInstantiatedInAnEnvironmentWithGemPath
as a parametrized tests.
* Add missing cases for Asciidoctor Factory in
WhenAnAsciidoctorClassIsInstantiatedInAnEnvironmentWithGemPath.

Fixes asciidoctor#1193
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.

2 participants