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

#641 Added tooling tests #880

Closed
wants to merge 3 commits into from

Conversation

crizzis
Copy link
Contributor

@crizzis crizzis commented Jun 10, 2022

No description provided.

@@ -56,7 +56,7 @@ class ArchUnitTestDescriptor extends AbstractArchUnitTestDescriptor implements C
private ClassCache classCache;

private ArchUnitTestDescriptor(ElementResolver resolver, Class<?> testClass, ClassCache classCache) {
super(resolver.getUniqueId(), testClass.getSimpleName(), ClassSource.from(testClass), testClass);
super(resolver.getUniqueId(), testClass.getName(), ClassSource.from(testClass), testClass);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this because Jupiter would group field-flavored and method-flavored test results under different parent nodes (one under FQ class name and the other under simple class name)

ext.minimumJavaVersion = JavaVersion.VERSION_1_9

sourceSets {
testFixtures {
Copy link
Contributor Author

@crizzis crizzis Jun 10, 2022

Choose a reason for hiding this comment

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

I couldn't just apply java-test-fixtures because then the source set gets added to the classpath in the form of a Jar. This messes up the test engines, which depend on file paths inside the resources directories

}
}

task removeFromMavenLocal {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This task cleans the local Maven repo to make sure we are running the Surefire and Gradle tests with an up-to-date version of ArchUnit jars

}

gradle.taskGraph.whenReady { graph ->
if (graph.hasTask(publishJUnitDepsToMavenLocal)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed because all the publishMavenJavaPublicationToMavenLocal tasks depend on test in their respective subprojects, so basically, publishJUnitDepsToMavenLocal would re-trigger the tests for all of those subprojects that we need in the local Maven repo.

Perhaps there's a better way of doing the same, didn't really investigate


@Override
protected String getBuildToolWrapperDirectory() {
return "gradle";
Copy link
Contributor Author

@crizzis crizzis Jun 10, 2022

Choose a reason for hiding this comment

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

The Gradle wrapper is not really used for now (note the explicit Gradle version specified inside GradleEngine)

Not sure which approach is better: to rely on wrappers inside testFixtures/resources, or to keep Gradle/Maven versions inside the test framework code (the former is arguably faster, since Gradle/Maven doesn't need to be re-downloaded each time)

import java.util.Map;
import java.util.stream.Stream;

public class BaselineTest extends BaseTest {
Copy link
Contributor Author

@crizzis crizzis Jun 10, 2022

Choose a reason for hiding this comment

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

This test isn't testing ArchUnit as such, it is only here to test the different test engines themselves (as a sort of sanity check).

It was useful during the test framework development - not sure if we want to keep it or not. Might still be useful if new test engines are introduced (e.g. for IDEs)


// Dependencies for tool integrations
surefireApi : [group: 'org.apache.maven.surefire', name: 'surefire-api', version: '3.0.0-M7'],
gradleApi : [group: 'dev.gradleplugins', name: 'gradle-api', version: '7.4.1'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gradleApi and byteBuddy are not yet used anywhere (they will be used for Gradle filtering)

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>
@crizzis crizzis force-pushed the feature/issue-641-tooling-tests branch from 8b0a3ad to 604c899 Compare June 10, 2022 15:07
@codecholeric
Copy link
Collaborator

Hey, thanks for all the hard work, but can you please put in a detailed description of why (I just see no description provided)? My problem here is: This adds 2.5K lines of code that we will have to maintain in the future. And I don't exactly see the benefit yet. In particular, what would be important to me is to weigh this against the simple system property filter suggested in the issue and then give detailed reasons why not to go the simple way.
Because I think we could add a simple filtering based on system properties that would solve the issue creators problem with a couple of hundred LOC for prod and test code altogether. And it would work with any tool, be it Gradle, Maven or IntelliJ, because system properties can always be supplied. So if we would go this way here, where we need this massive complexity of test infrastructure, then we need to state a good reason for it IMHO. So can you outline this? What is your vision? Why do we need these tooling tests for it? What would we gain over the simple filtering mechanism?

Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>
Signed-off-by: Krzysztof Sierszeń <krzysztof.sierszen@digitalnewagency.com>
@codecholeric
Copy link
Collaborator

I implemented a simplified version in #1280. Since I have no capacity to work on this further, I'm gonna close this. Feel free to reopen if you want to pick this up or add further improvements.

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

Successfully merging this pull request may close these issues.

2 participants