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

Support the selection of a default Runner in ClassRequest #1423

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cesar1000
Copy link

This patch updates Request and ClassRequest to enable the configuration of a default JUnit4 Runner other than BlockJUnit4ClassRunner. The only visible change is a new static method in Request, classWithDefaultRunner, which configures the runner class to use if not configured in the test.

The context of this change is the following: when managing a large test suite that gets contributions from a large development team, it's typical to enforce certain logic across all tests for global state initialization and cleanup. In the particular case of Twitter for Android, all our tests set up global application graphs before test execution and reset static state afterwards. It is critical for correctness that every single test follows this pattern. This seems to be a particular example of the general need for a mechanism that configures the global state of the test environment.

This is typically achieved by forcing all tests to use a single shared runner or to extend a base class. However, these approaches are error prone, since they require dilligence for setting up the test correctly, or an external validation (eg. a checkstyle detector). A default test runner is a low-tech solution to this problem: a test runner extending BlockJUnit4ClassRunner implements state management logic and is configured to be used with all tests that don't explicitly say otherwise.

#1219 proposes an alternative solution to a similar problem. I feel that this approach covers the general need in a simple, non-intrusive way.

The change is incomplete and doesn't implement tests, and it's meant a small proof of concept for feedback.

This patch updates `Request` and `ClassRequest` to enable the configuration of a default JUnit4 Runner other than `BlockJUnit4ClassRunner`. The only visible change is a new static method in Request, `classWithDefaultRunner`, which configures the runner class to use if not configured in the test.

The context of this change is the following: when managing a large test suite that gets contributions from a large development team, it's typical to enforce certain logic across all tests for global state initialization and cleanup. In the particular case of Twitter for Android, all our tests set up global application graphs before test execution and reset static state afterwards. It is critical for correctness that every single test follows this pattern. This seems to be a particular example of the general need for a mechanism that configures the global state of the test environment.

This is typically achieved by forcing all tests to use a single shared runner or to extend a base class. However, these approaches are error prone, since they require dilligence for setting up the test correctly, or an external validation (eg. a checkstyle detector). A default test runner is a low-tech solution to this problem: a test runner extending `BlockJUnit4ClassRunner` implements state management logic and is configured to be used with all tests that don't explicitly say otherwise.

 The change is incomplete and doesn't implement tests, and it's meant a small proof of concept for feedback.
@kcooney
Copy link
Member

kcooney commented Feb 27, 2017

Thanks for the contribution.

My concern with this approach is that if you run tests in a way that won't call this new method (ex: from an IDE) you would get different results. This could lead to confusion. Having your tests explicitly specify a runner avoids this problem.

@cesar1000
Copy link
Author

Thanks, Kevin!

Yeah, that's a good point. Ideally, the information should be available in code. Would you consider something like looking up a @RunWith annotation applied to a package? In the case that the test is not annotated, the annotation runner would look up the chain of parent packages for the annotation. Implementers can then either add the package info file to the codebase or generate dynamically at compilation time - in either case, the configuration would be picked up by all test execution methods.

@kcooney
Copy link
Member

kcooney commented Feb 28, 2017

@cesar1000 see #670 for a related discussion.

One open question is what to do if your test class extends a test class in a different package and neither test class has @RunWith? My gut tells be you should use the ((great) grand) parent test clasa's package to find the runner because you are extending the base class, so should inherit the base class's runner (because the runner is part of the behavior of the base class).

@cesar1000
Copy link
Author

Ah, thanks for the pointer. Yeah, I feel that the meaning of @RunWith or @Ignore when applied to a package would be well defined. I can see the point that developers may not expect annotations in packages, but I think there are strong practical reasons why teams may decide to use them, since JUnit does not offer alternative tools for applying a certain configuration to a set of tests.

In regard to selecting a runner when extending a test class: I think we should always use the test's package. On the one hand, this is a simple and consistent rule that makes it easy to find the runner. On the other hand, I think it's the responsibility of the implementer of a test to ensure that the test runner is consistent with its parent's (as you would normally do if you were to annotate your test to use a different runner). Do you have any particular case in mind?

If this sounds good, I can throw a prototype together to see how that looks and we can discuss.

@cesar1000
Copy link
Author

I hit a snag with using package annotations: it appears that Java will only initialize Package objects when first loading a class from the package. This is ok if annotating the package containing the tests, but won't work with parent packages (unless adding placeholder classes to these packages and loading them explicitly, but that's a bad hack). Annotating packages containing tests can be useful in itself, but it feels incomplete and error prone, since people are likely to expect this feature to work on any package. Alternatively, we could make the annotation apply only to the package itself and not subpackages, but this limits its usefulness.

An alternative may be to define a named class (JUnitPackageConfiguration or similar) and load that by name. The class would define annotations or other configuration parameters. Would this be an acceptable solution? Do you have any other ideas?

@kcooney
Copy link
Member

kcooney commented Mar 5, 2017

@cesar1000 I think a named package would be even less discoverable to a layperson than using package-level annotations.

@cesar1000
Copy link
Author

@kcooney yeah, I agree. I was just hoping for a more powerful solution, but I understand that discoverability is an important goal. It's a shame that Java support for package annotations is so inconsistent. Let me give it a bit more thought.

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.

2 participants