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

Testing package dependencies with Degraph #71

Closed
wants to merge 12 commits into from
Closed

Testing package dependencies with Degraph #71

wants to merge 12 commits into from

Conversation

@schauder
Copy link
Contributor

@schauder schauder commented Dec 19, 2015

Adds a test that tests for dependency cycles on package an module level.

Modules in this sense are all classes that start with the same package: org.junit.gen5.x

In order to make the test pass I had to break to dependencies from test to the JUnit5TestEngine. Since the engine wasn't really part of the test, I replaced it with a mock.

I agree to https://github.com/junit-team/junit-lambda/blob/master/CONTRIBUTING.md

schauder added 9 commits Dec 19, 2015
The test currently fails due to a dependency
from org.junit.gen5.engine.junit5.descriptor
to org.junit.gen5.engine.junit5
In Tests, while there exists plenty of dependencies
in the other direction in other code
The dependency caused a dependency circle on package level
The Test wasn't really related to the JUnit5TestEngine, it just needed an Engine with an id.
The dependency caused a dependency circle on package level
The Test wasn't really related to the JUnit5TestEngine, it just needed an Engine with an id.
The checks between modules is somewhat redundant, since it should already be enforced by code
conventions plus gradle.

Also added a comment for the test.
Gradle includes dependencies to other modules as jar files, so Degraph won't see them if we tell it to ignore jars.
removing the noJar() enabled analysis of JUnit classes.
Fixed this by making the include more strict.

Also tried to find a compromise between readable code layout and what spotlessApply does
@codecov-io
Copy link

@codecov-io codecov-io commented Dec 19, 2015

Current coverage is 67.31%

Merging #71 into master will not affect coverage as of 96624d5

@@            master     #71   diff @@
======================================
  Files           94      94       
  Stmts         2163    2163       
  Branches       249     249       
  Methods          0       0       
======================================
  Hit           1456    1456       
  Partial         75      75       
  Missed         632     632       

Review entire Coverage Diff as of 96624d5

Powered by Codecov. Updated on successful CI builds.


testRuntime("org.apache.logging.log4j:log4j-core:${log4JVersion}")
testRuntime("org.apache.logging.log4j:log4j-jul:${log4JVersion}")

This comment has been minimized.

@marcphilipp

marcphilipp Dec 19, 2015
Member

Why did you remove that?

This comment has been minimized.

@schauder

schauder Dec 19, 2015
Author Contributor

Ooops, by accident. I'll put it back in.

@marcphilipp
Copy link
Member

@marcphilipp marcphilipp commented Dec 19, 2015

@schauder Thanks for the PR! Just one minor question in the diff.

* http://www.eclipse.org/legal/epl-v10.html
*/

package org.junit.gen5.commons.util.org.junit.gen5.meta;

This comment has been minimized.

@marcphilipp

marcphilipp Dec 19, 2015
Member

Should be just org.junit.gen5.meta, right?

@marcphilipp
Copy link
Member

@marcphilipp marcphilipp commented Dec 19, 2015

@schauder Is there a way to tell degraph that org.junit.gen5.engine.junit4 (junit4-engine), org.junit.gen5.engine.junit5 (junit5-engine) and everything else in org.junit.gen5.engine.** (engine-api) are separate modules? I tried using NamedPattern but couldn't get it to work.

@marcphilipp marcphilipp self-assigned this Dec 19, 2015
@marcphilipp marcphilipp added this to the Alpha 1 milestone Dec 19, 2015
@jlink
Copy link
Contributor

@jlink jlink commented Dec 19, 2015

For legal reasons you'll have to add a paragraph to the pull request description that states that you agree to the terms in https://github.com/junit-team/junit-lambda/blob/master/CONTRIBUTING.md

@schauder
Copy link
Contributor Author

@schauder schauder commented Dec 19, 2015

@marcphilipp it is possible and NamedPatterns are the way to go (although you could achieve it without them) You probably used the parameters in the wrong oder. No idea why I put them the way they are ...

See the latest commit.

@schauder
Copy link
Contributor Author

@schauder schauder commented Dec 19, 2015

@jlink done. Pulling out org.junit.Assert right now and replacing it with import static org.hamcrest.MatcherAssert.assertThat;

@marcphilipp
Copy link
Member

@marcphilipp marcphilipp commented Dec 19, 2015

@schauder Thanks! I've merged this pull request manually. Great to finally have this check in place. 👍

@jlink
Copy link
Contributor

@jlink jlink commented Dec 21, 2015

I'd like to pull DependencyTests out of junit-tests since it takes 13 secs and slows down the suite.
Any counter arguments? Pull out to a new project?

@sbrannen
Copy link
Member

@sbrannen sbrannen commented Dec 21, 2015

I agree with @jlink: DependencyTests should not be a part of the standard pre-commit build. In fact, I think it shouldn't be part of the standard CI build either. Rather, I would prefer that it be part of a nightly build.

@schauder
Copy link
Contributor Author

@schauder schauder commented Dec 21, 2015

While I agree that it is to slow for the first build step of a CI pipeline, I prefer to let it run after every push, but in a separate build step.

Another alternative would be to include such a test in each project. Since it would only run on the classes (not the jar files) it should be much faster, although still not as fast as one would wish for a proper Unit test.

@jlink
Copy link
Contributor

@jlink jlink commented Dec 21, 2015

I don't really mind to have it as a precommit step or in the ci build. But I also want a very fast test suite to run (more or less) continuously.

@schauder Did you think about externalizing the use of the tool into a gradle/maven plugin that gets a DSL like configuration block? This would give users the freedom to use it in the way best fit for their project setup.

@schauder
Copy link
Contributor Author

@schauder schauder commented Dec 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants