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

Introduce configurable discovery algorithm #1798

Merged
merged 1 commit into from Mar 7, 2019

Conversation

@marcphilipp
Copy link
Member

commented Mar 3, 2019

The EngineDiscoveryRequestResolver class implements a test discovery
in a way that can be reused by different test engines. The Jupiter and
Vintage test engines are changed to use it.

Main entry point is EngineDiscoveryRequestResolver.builder() which
allows to add resolvers (via a new SelectorResolvers interface) and
visitors (reusing TestDescriptor.Visitor) that will be called to
resolve DiscoverySelectors in a given EngineDiscoveryRequest.

In order to support class based test engines, the predefined
ClassContainerSelectorResolver may be added via the builder as well.
It resolves ClasspathRootSelectors, ModuleSelectors, and
PackageSelectors into ClassSelectors by scanning for classes in the
corresponding class container.

The fundamental principle behind SelectorResolver is that both the
designated parent of a TestDescriptor and its designated children can
be specified using DiscoverySelectors. For example, in the case of the
Jupiter engine, when resolving a nested test class, there's a resolver
that specifies the parent using the ClassSelector of the enclosing
class and the children as the set of MethodSelectors of the testable
methods it contains joined with the set of ClassSelectors of nested
classes it contains.

Overall, the algorithm works as follows:

  1. Enqueue all selectors in the supplied request to be resolved.
  2. While there are selectors to be resolved, get the next one.
    Otherwise, the resolution is finished.
    1. Iterate over all registered SelectorResolvers in the order they
      were registered in and find the first one that returns a non-empty
      resolution.
    2. If such a resolution exists, enqueue the contained selectors.
    3. For each exact match in the resolution, expand its children and
      enqueue them as well.
    4. Iterate over all registered visitors and let the engine descriptor
      accept them.

Descriptors with the same unique ID are only resolved once, even if
specified using multiple selectors.

Resolves #1739.

Overview

Please describe your changes here and list any open questions you might have.


I hereby agree to the terms of the JUnit Contributor License Agreement.


Definition of Done

@marcphilipp marcphilipp added this to the 5.5 M1 milestone Mar 3, 2019

@marcphilipp marcphilipp self-assigned this Mar 3, 2019

@ghost ghost added the status: reviewing label Mar 3, 2019

@jbduncan
Copy link
Contributor

left a comment

Just a couple of minor suggestions/queries from me. :)

.map(DiscoverySelectors::selectClass);
return Stream.concat(methods, nestedClasses).collect(toCollection((Supplier<Set<DiscoverySelector>>) LinkedHashSet::new));
}));
// @formatter:on

This comment has been minimized.

Copy link
@jbduncan

jbduncan Mar 3, 2019

Contributor

@marcphilipp It looks to me that the code within this @formatter:(off|on) block has some space/tab indenting issues. :)

This comment has been minimized.

Copy link
@sormuras

sormuras Mar 3, 2019

Member

/me mumbles Turning formatters off in 2019... 😉

This comment has been minimized.

Copy link
@jbduncan

jbduncan Mar 3, 2019

Contributor

@sormuras Indeed. 😉

But on a more serious note, it makes me wonder if Eclipse has a setting that would allow method chains to be formatted with one method per line, without the JUnit 5 team having to manually use @formatter(off|on)... 🤔

(Am I right to think that using google-java-format, which IIUC would solve this problem more easily, was considered by the JUnit 5 team but decided against?)

This comment has been minimized.

Copy link
@sormuras

sormuras Mar 4, 2019

Member

... using google-java-format ... was considered by the JUnit 5 team but decided against?

True. But I'm only allowed to bring up that topic once a year. Mentioned it last week already -- so when is 2020?! 😈

This comment has been minimized.

Copy link
@marcphilipp

marcphilipp Mar 7, 2019

Author Member

@jbduncan I don't see any space/tab indenting issues. What am I missing?

This comment has been minimized.

Copy link
@sormuras

This comment has been minimized.

Copy link
@sormuras

sormuras Mar 7, 2019

Member

Could be an GitHub display only issue. Didn't check the whitespace chars in my IDE...

This comment has been minimized.

Copy link
@jbduncan

jbduncan Mar 9, 2019

Contributor

Indeed it could be just a GitHub issue; I don't have an IDE with this PR checked out to confirm. 😅

This comment has been minimized.

Copy link
@sbrannen

sbrannen Mar 10, 2019

Member

Nope: not a GitHub. They were spaces instead of tabs. Fixed here: f114fd0 😉

This comment has been minimized.

Copy link
@jbduncan

jbduncan Mar 10, 2019

Contributor

Haha, thank you for confirming and cleaning it up, @sbrannen. 😄

private final TestDescriptor engineDescriptor;
private final Map<DiscoverySelector, Resolution> resolvedSelectors = new LinkedHashMap<>();
private final Map<UniqueId, Match> resolvedUniqueIds = new LinkedHashMap<>();
private final Queue<DiscoverySelector> remainingSelectors = new LinkedList<>();

This comment has been minimized.

Copy link
@jbduncan

jbduncan Mar 3, 2019

Contributor

Have you considered using an ArrayDeque instead of a LinkedList here?

This comment has been minimized.

Copy link
@marcphilipp

marcphilipp Mar 7, 2019

Author Member

I haven't TBH. Do you think it would make a difference in this scenario performance-wise?

This comment has been minimized.

Copy link
@jbduncan

jbduncan Mar 9, 2019

Contributor

I'm not sure, actually. I personally prefer ArrayDeque because its Javadoc says that it's "faster than LinkedList when used as a queue", so when in doubt, that's the implementation I go for.

But having said this, I don't know how ArrayDeque using an array for storage would impact its memory consumption on a long-running instance of the JUnit Platform (as in, I don't know (1) if the queue will need to live for a "long" time, and (2) whether it's possible for the queue to get very long and then empty, causing a memory leak if the array isn't resized back to a small size).

So I'd say that using a LinkedList here is perfectly acceptable until we have evidence that it's a performance bottleneck or a memory hog. :)

This comment has been minimized.

Copy link
@marcphilipp

marcphilipp Mar 10, 2019

Author Member

I've now opted for ArrayDeque for the reason you mentioned. The queue is short-lived and will only live for the duration of test discovery so we shouldn't have to worry about it being a memory leak.

This comment has been minimized.

Copy link
@jbduncan

jbduncan Mar 10, 2019

Contributor

Sounds good to me @marcphilipp! Cheers. 👍

@ghost ghost assigned sormuras Mar 3, 2019

@codecov

This comment has been minimized.

Copy link

commented Mar 4, 2019

Codecov Report

Merging #1798 into master will decrease coverage by 0.09%.
The diff coverage is 92.22%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master    #1798     +/-   ##
===========================================
- Coverage     91.71%   91.61%   -0.1%     
+ Complexity     4113     4081     -32     
===========================================
  Files           350      342      -8     
  Lines          9715     9736     +21     
  Branches        779      794     +15     
===========================================
+ Hits           8910     8920     +10     
- Misses          616      624      +8     
- Partials        189      192      +3
Impacted Files Coverage Δ Complexity Δ
...jupiter/engine/descriptor/ClassTestDescriptor.java 96.91% <ø> (ø) 64 <0> (ø) ⬇️
...ntage/engine/descriptor/VintageTestDescriptor.java 96.55% <ø> (ø) 28 <0> (ø) ⬇️
...r/engine/descriptor/TestFactoryTestDescriptor.java 94.64% <ø> (ø) 20 <0> (ø) ⬇️
.../engine/descriptor/TestTemplateTestDescriptor.java 100% <ø> (ø) 16 <0> (ø) ⬇️
...tage/engine/discovery/IgnoringRunnerDecorator.java 100% <ø> (ø) 4 <0> (ø) ⬇️
...r/engine/descriptor/NestedClassTestDescriptor.java 100% <ø> (ø) 4 <0> (ø) ⬇️
.../org/junit/vintage/engine/descriptor/OrFilter.java 100% <ø> (ø) 3 <0> (?)
...er/engine/descriptor/TestMethodTestDescriptor.java 96.87% <ø> (ø) 43 <0> (ø) ⬇️
...er/engine/discovery/DiscoverySelectorResolver.java 100% <100%> (ø) 7 <6> (ø) ⬇️
...e/discovery/RunnerTestDescriptorPostProcessor.java 100% <100%> (ø) 10 <2> (?)
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e41ebe...5371619. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Mar 4, 2019

Codecov Report

Merging #1798 into master will decrease coverage by 0.1%.
The diff coverage is 92.22%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #1798      +/-   ##
===========================================
- Coverage     91.71%   91.6%   -0.11%     
+ Complexity     4113    4081      -32     
===========================================
  Files           350     342       -8     
  Lines          9715    9736      +21     
  Branches        779     794      +15     
===========================================
+ Hits           8910    8919       +9     
- Misses          616     624       +8     
- Partials        189     193       +4
Impacted Files Coverage Δ Complexity Δ
...jupiter/engine/descriptor/ClassTestDescriptor.java 96.91% <ø> (ø) 64 <0> (ø) ⬇️
...ntage/engine/descriptor/VintageTestDescriptor.java 96.55% <ø> (ø) 28 <0> (ø) ⬇️
...r/engine/descriptor/TestFactoryTestDescriptor.java 94.64% <ø> (ø) 20 <0> (ø) ⬇️
.../engine/descriptor/TestTemplateTestDescriptor.java 100% <ø> (ø) 16 <0> (ø) ⬇️
...tage/engine/discovery/IgnoringRunnerDecorator.java 100% <ø> (ø) 4 <0> (ø) ⬇️
...r/engine/descriptor/NestedClassTestDescriptor.java 100% <ø> (ø) 4 <0> (ø) ⬇️
.../org/junit/vintage/engine/descriptor/OrFilter.java 100% <ø> (ø) 3 <0> (?)
...er/engine/descriptor/TestMethodTestDescriptor.java 96.87% <ø> (ø) 43 <0> (ø) ⬇️
...er/engine/discovery/DiscoverySelectorResolver.java 100% <100%> (ø) 7 <6> (ø) ⬇️
...e/discovery/RunnerTestDescriptorPostProcessor.java 100% <100%> (ø) 10 <2> (?)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e41ebe...9ffd427. Read the comment docs.

@sormuras
Copy link
Member

left a comment

LGTM

Introduce configurable discovery algorithm
The `EngineDiscoveryRequestResolver` class implements a test discovery
in a way that can be reused by different test engines. The Jupiter and
Vintage test engines are changed to use it.

Main entry point is `EngineDiscoveryRequestResolver.builder()` which
allows to add resolvers (via a new `SelectorResolvers` interface) and
visitors (reusing `TestDescriptor.Visitor`) that will be called to
resolve `DiscoverySelectors` in a given `EngineDiscoveryRequest`.

In order to support class based test engines, the predefined
`ClassContainerSelectorResolver` may be added via the builder as well.
It resolves `ClasspathRootSelectors`, `ModuleSelectors`, and
`PackageSelectors` into `ClassSelectors` by scanning for classes in the
corresponding class container.

The fundamental principle behind `SelectorResolver` is that both the
designated parent of a `TestDescriptor` and its designated children can
be specified using `DiscoverySelectors`. For example, in the case of the
Jupiter engine, when resolving a nested test class, there's a resolver
that specifies the parent using the `ClassSelector` of the enclosing
class and the children as the set of `MethodSelectors` of the testable
methods it contains joined with the set of `ClassSelectors` of nested
classes it contains.

Overall, the algorithm works as follows:

1. Enqueue all selectors in the supplied request to be resolved.
2. While there are selectors to be resolved, get the next one.
   Otherwise, the resolution is finished.
   a. Iterate over all registered `SelectorResolvers` in the order they
      were registered in and find the first one that returns a non-empty
      resolution.
   b. If such a resolution exists, enqueue the contained selectors.
   c. For each exact match in the resolution, expand its children and
      enqueue them as well.
   d. Iterate over all registered visitors and let the engine descriptor
      accept them.

Descriptors with the same unique ID are only resolved once, even if
specified using multiple selectors.

Resolves #1739.

@marcphilipp marcphilipp force-pushed the issues/1739-reusable-discovery branch from 5371619 to 9ffd427 Mar 7, 2019

@marcphilipp marcphilipp merged commit 9ffd427 into master Mar 7, 2019

1 of 5 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
WIP Ready for review
Details

@ghost ghost removed the status: reviewing label Mar 7, 2019

@marcphilipp marcphilipp deleted the issues/1739-reusable-discovery branch Mar 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.