Skip to content

Part II - Add module org.junit.platform.commons.jpms #1057

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Part II - Add module org.junit.platform.commons.jpms #1057

wants to merge 1 commit into from

Conversation

sormuras
Copy link
Member

@sormuras sormuras commented Sep 11, 2017 •
edited
Loading

Overview

This PR introduces org.junit.platform.commons.jpms as the default ModuleClassFinder implementation. See #1061 for details.

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

Definition of Done

@sormuras sormuras added this to the 5.1 Backlog milestone Sep 11, 2017
@sormuras sormuras self-assigned this Sep 11, 2017
@sormuras
Copy link
Member Author

sormuras commented Sep 11, 2017 •
edited
Loading

I started to work on a proof-of-concept service implementation at https://github.com/sormuras/sawdust/tree/master/module-discoverer/discoverer/src

Edit: that project was superseded by the junit-platform-common-jpms sub-project in this PR.

marcphilipp
marcphilipp reviewed Sep 12, 2017
View reviewed changes
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.

Looks good in general!

build.gradle Outdated
def signArtifacts = !project.version.contains('SNAPSHOT')
def isSnapshot = project.version.contains('SNAPSHOT')
def isContinuousIntegrationEnvironment = Boolean.parseBoolean(System.getenv("JITPACK"))
def signArtifacts = !(isSnapshot || isContinuousIntegrationEnvironment)
Copy link
Member

Choose a reason for hiding this comment

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

Didn't you want to sign the artifacts for Jitpack?

Copy link
Member Author

Choose a reason for hiding this comment

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

It fails on JitPack as there are no credentials available over there.

The purpose, namely depending on a branch SNAPSHOT build, is served well w/o signing and usually, one could use our official master SNAPSHOT distro anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, makes sense, sorry I misremembered our conversation about signing.

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ModuleUtils.java Outdated
/**
* Class finder service providing interface.
*/
public interface ClassFinder {
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 a top-level class, e.g. ModuleClassFinder, would make it easier to use because it wouldn't require a $ in the file name of implementors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mh, thought about that too.

But a) it's an internal SPI and b) you don't specify the $ when using it in a module descriptor:

module org.junit.platform.commons.jpms {
  requires org.junit.platform.commons;

  provides org.junit.platform.commons.util.ModuleUtils.ClassFinder with
      org.junit.platform.commons.jpms.ModuleClassFinder;
}

junit-platform-commons/src/main/java/org/junit/platform/commons/util/ModuleUtils.java Outdated
List<Class<?>> classes = new ArrayList<>();
for (ClassFinder classFinder : ServiceLoader.load(ClassFinder.class, classLoader)) {
classes.addAll(classFinder.findAllClassesInModule(moduleName, classTester, classNameFilter));
}
Copy link
Member

Choose a reason for hiding this comment

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

Shall we throw an exception or at least log something when no ClassFinder is registered?

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally yes. Should have added the TODO marker here that exists in mind...

@sormuras
Copy link
Member Author

Current command line reads:

java
--module-path
  build\target\mods;build\bach\resolved
--add-modules
  ALL-MODULE-PATH
--module
  org.junit.platform.console
--select-module
  jpms.integration
.
'-- JUnit Jupiter [OK]
  '-- JpmsIntegrationTests [OK]
    +-- insideExplicitModule() [OK]
    '-- greetings() [OK]
      '-- hello [OK]

Test run finished after 30 ms
[         3 containers found      ]
[         0 containers skipped    ]
[         3 containers started    ]
[         0 containers aborted    ]
[         3 containers successful ]
[         0 containers failed     ]
[         2 tests found           ]
[         0 tests skipped         ]
[         2 tests started         ]
[         0 tests aborted         ]
[         2 tests successful      ]
[         0 tests failed          ]

@sbrannen sbrannen mentioned this pull request Sep 13, 2017
Closed
8 tasks
@sormuras sormuras changed the title Add module selection support Part II - Add module org.junit.platform.commons.jpms Sep 13, 2017
@sormuras sormuras mentioned this pull request Sep 13, 2017
Closed
8 tasks
@sormuras sormuras force-pushed the jpms branch 3 times, most recently from 6a4ad56 to a4d9e48 Compare September 15, 2017 08:12
@sormuras
Copy link
Member Author

Moved to a 100% Gradle build setup -- with a bunch of manual tasks to show-case "same module" and "integration" testing.

@sormuras
Copy link
Member Author

sormuras commented Sep 15, 2017 •
edited
Loading

:install fails atm with

Execution failed for task ':junit-platform-commons-jpms:install'.
> Could not publish configuration 'archives'
   > Could not write to file 'C:\Dev\Github\junit-team\junit5\junit-platform-commons-jpms\build\poms\pom-default.xml'.

[...]

The configuration to scope mapping is not unique.
The following configurations have the same priority: [org.gradle.api.artifacts.maven.Conf2ScopeMapping@72b19c97,
org.gradle.api.artifacts.maven.Conf2ScopeMapping@2661890c]

See also: https://jitpack.io/com/github/junit-team/junit5/jpms-r5.0.0-g2b9b12d-11/build.log

@sormuras sormuras force-pushed the jpms branch 2 times, most recently from b94eb48 to 2703730 Compare September 18, 2017 07:09
@sormuras
Introduce sub-project junit-platform-commons-jpms
eba6164 
This commit contains the default ModuleClassFinder implementation that
scans Java 9 modules for testable classes. Therefore you need JDK 9 to
build this sub-project.

Addresses #425
@sormuras sormuras mentioned this pull request Sep 28, 2017
Closed
3 tasks
@sormuras
Copy link
Member Author

sormuras commented Oct 7, 2017

Superseded by #1087

@sormuras sormuras closed this Oct 7, 2017
@ghost ghost removed the status: in progress label Oct 7, 2017
@sormuras sormuras deleted the jpms branch October 17, 2017 12:45
@marcphilipp marcphilipp removed this from the 5.x Backlog milestone Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcphilipp marcphilipp marcphilipp left review comments

Assignees

@sormuras sormuras

Labels
component: Jupiter component: Platform theme: discovery theme: modules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sormuras @marcphilipp