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

[WIP] Add tag and test source support to dynamic containers and tests #1396

Closed
wants to merge 1 commit into from

Conversation

sormuras
Copy link
Member

Overview

Addresses #1178


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


Definition of Done

@codecov
Copy link

codecov bot commented Apr 28, 2018

Codecov Report

Merging #1396 into master will decrease coverage by 0.21%.
The diff coverage is 64%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1396      +/-   ##
============================================
- Coverage     92.05%   91.84%   -0.22%     
- Complexity     3257     3259       +2     
============================================
  Files           301      301              
  Lines          7846     7868      +22     
  Branches        662      663       +1     
============================================
+ Hits           7223     7226       +3     
- Misses          453      473      +20     
+ Partials        170      169       -1
Impacted Files Coverage Δ Complexity Δ
...n/java/org/junit/jupiter/api/DynamicContainer.java 53.84% <0%> (-46.16%) 4 <0> (ø)
...c/main/java/org/junit/jupiter/api/DynamicNode.java 88.88% <100%> (+8.88%) 4 <3> (+2) ⬆️
...r/engine/descriptor/TestFactoryTestDescriptor.java 93.87% <100%> (+0.26%) 15 <5> (ø) ⬇️
...ine/descriptor/DynamicContainerTestDescriptor.java 100% <100%> (ø) 6 <1> (ø) ⬇️
...r/engine/descriptor/DynamicNodeTestDescriptor.java 100% <100%> (ø) 5 <1> (+1) ⬆️
...r/engine/descriptor/DynamicTestTestDescriptor.java 100% <100%> (ø) 3 <1> (ø) ⬇️
...c/main/java/org/junit/jupiter/api/DynamicTest.java 81.25% <50%> (-18.75%) 6 <1> (+1)
...rams/aggregator/ArgumentsAggregationException.java 0% <0%> (-50%) 0% <0%> (-1%)
...ter/params/ParameterizedTestParameterResolver.java 75% <0%> (-22.37%) 15% <0%> (-1%)
...er/params/aggregator/DefaultArgumentsAccessor.java 96.42% <0%> (-3.58%) 18% <0%> (-1%)
... and 1 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 0d68ca6...b6cd523. Read the comment docs.

Copy link
Member

@marcphilipp marcphilipp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the use case for specifying a TestSource, I'm not sure how adding tags would be useful. The main use case for tags is to the tests that should be executed. However, we only apply those filters to static TestDescriptors at the moment.

UniqueId uniqueId;
Supplier<JupiterTestDescriptor> descriptorCreator;
TestSource source = node.getTestSource().orElse(testSource);
Set<TestTag> tags = node.getTags().stream().map(TestTag::create).collect(Collectors.toSet());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need some exception handling here because TestTag::new might throw an exception.

@@ -113,4 +116,16 @@ public Executable getExecutable() {
return this.executable;
}

public DynamicTest withTags(String... tags) {
DynamicTest result = new DynamicTest(getDisplayName(), executable);
result.tags = new HashSet<>(Arrays.asList(tags));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think calling withTags() multiple times should add the tags instead of overwriting the existing ones.

@@ -2,7 +2,8 @@ description = 'JUnit Jupiter API'

dependencies {
api("org.opentest4j:opentest4j:${ota4jVersion}")
api(project(":junit-platform-commons"))
api(project(':junit-platform-commons'))
compileOnly(project(':junit-platform-engine'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we all agree to go this route, we should make sure junit-platform-engine is listed as optional in the generated POM.

@robeden
Copy link

robeden commented May 4, 2018

@marcphilipp - We discussed the theoretical use case* for tags from DynamicTests in #1178. Of course, that all relies on them being honored for test filtering/running/skipping. If that doesn't work due to where DynamicTests are created in the lifecycle, then I agree that they're not particularly useful.

* - Basically, allows a lot of benefit of a new TestEngine with less work in implementation.

@sormuras
Copy link
Member Author

Superseded by #1428

@sormuras sormuras closed this May 22, 2018
@ghost ghost removed the status: in progress label May 22, 2018
@sormuras sormuras deleted the issues/1178-with-tags-and-source branch June 14, 2018 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants