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

Add JUnit5 implementation of CompilationRule #155

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Kiskae
Copy link

@Kiskae Kiskae commented Oct 7, 2018

This commit adds a JUnit5 extension based on the existing CompilationRule

Because of the difference between JUnit4 Rule and JUnit5 Extension it is not a straightforward conversion, but the tests are just a straight copy since the primary difficulty exists in the extension itself due to the asynchronous nature of JUnit5:

  • Elements and Types are only available inside of a Processor. Because JUnit5 does not allow extensions to take direct control of the execution of tests this required a workaround where the compiler is executed on a separate thread and then blocked while the tests are running, passing over the ProcessingEnvironment during that span of time.
  • The extension uses ParameterResolver to inject Elements and Types directly as parameters to the method when they are required. This should also be compatible with other parameter injecting extensions like @ParameterizedTest. Accessing them as methods like CompilationRule does would not work since JUnit5 strongly recommends that all extensions store their state inside of ExtensionContext.Store, which the tests themselves cannot directly access.
  • Unlike CompilationRule the extension runs a single compilation for the entire set of tests. Since the input of the compiler does not change I believe this would not lead to invalid results. If running a compilation for each separate test is required then the only change that needs to be made is replacing (Before/After)All with (Before/After)Each.
    • As of commit bcf955a it will fall back automatically if the class-level (Before/After)All are not being used by the user. This happens if @ExtendWith is applied to a method directly or when @RegisterExtension is used.
  • The JUnit5 api dependency is marked as optional so it does not clutter the classpath when users do not use JUnit5. I'd apply this to the existing JUnit4 dependency as well but that would be a breaking change.

The only questionable part is the finalization of the compilation which currently relies on a call to CompletableFuture.get(long,TimeUnit) with a timeout of 1 second. I'm not entirely sure if the timeout could potentially be reached during a normal execution or if there is a better way to handle that part of the synchronization.

Kiskae and others added 7 commits October 7, 2018 19:27
JUnit5 provides a specific exception for parameter resolution failures
If the compilation fails before the EvaluatingProcessor blocks then the
extension would be permanently waiting. Forcing termination on
completion of the compilation will ensure it can never be stuck waiting.
- Refactor so in absence of (before/after)All lifecycle events, it falls
  back on the (before/after)Each events. This supports
  @RegisterExtension and per-method @ExtendWith usage scenarios.
- Slim down EvaluatingProcessor so it can either fail to the
  CompletableFuture or block on the Phaser, simplifying the exception
  path.
- Extract all state in a dedicated object which can do checks on the
  state of the phaser before actions are performed.
- Add tests for the compilation state and the various operations that
  synchronize it with the tests.
- Test each of the different extension usage scenarios.
  - @ExtendWith on class
  - @ExtendWith on method
  - @RegisterExtension with Lifecycle.PER_CLASS
  - @RegisterExtension with Lifecycle.PER_METHOD
@Kiskae
Copy link
Author

Kiskae commented Oct 9, 2018

As mentioned in bcf955a, I've reworked the extension for two reasons:

  1. Simplify the internal annotation processor so it can either succeed and block waiting for tests, or fail and cause the resulting exception to be thrown before any tests are run.
  2. To support all ways that an extension can be used by the user. If the way the user uses the extension means the (before/after)All events are skipped, then just fall back on running the compiler for each individual test. CompilationExtensionTest has been changed to test all 4 ways the extension can apply to a test class.

@ptahchiev
Copy link

+1 on this PR. I would like to see it merged so we can finally start using JUnit5.

@ronshapiro
Copy link
Contributor

Google doesn't use JUnit 5 at all, and so we wouldn't have any way of testing/maintaining this. This may be worthwhile as a standalone library instead.

@Kiskae
Copy link
Author

Kiskae commented Dec 23, 2018

I've moved the extension to its own repository at https://github.com/Kiskae/compile-testing-extension and submitted a request to include it in jcenter. I'll leave it to the repo owners to close this PR or change it to just link to the third-party resource as a reference.

@ptahchiev
Copy link

Several things

  • This issue will not be resolved even with this PR merged. It is blocked by https://github.com/google/truth/issues/333is
  • I think this is the right place for this pull-request. Google not using junit5 internally does not seem like a valid excuse. This is a useful project for a lot of people - not just google. If you are going to include features that are only valid for Google, then what's the point of open-sourcing it?!

@ronshapiro
Copy link
Contributor

If we have no way to test, or validate the implementation, or have no experience with JUnit 5, it doesn't seem like us being the owners of the code seems to make sense. There can be many libraries that work well together without being one conglomerate. One exception to this, I would think, is if the internals of the Compile Testing are critical to the implementation of the code, which this isn't.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes P4 type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants