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

Interfaces syntax #36

Merged
merged 5 commits into from
Nov 4, 2017
Merged

Interfaces syntax #36

merged 5 commits into from
Nov 4, 2017

Conversation

hajotka
Copy link

@hajotka hajotka commented Oct 19, 2017

This adds are(Not)Interfaces and (not)beInterfaces.

The should().onlyBeAccessed().byClassesThat().are*Interfaces() doesn't make sense in Java 7, only with Java 8 with default methods. I added the relevant tests and tried them with Java 8, however currently they are ignored. See TODOs in ShouldOnlyBeAccessedByClassesThatTest.

Accessing a constant from an interface does not count as an access - is this as intended? Also see ShouldOnlyBeAccessedByClassesThatTest.java.

I hereby agree to the terms of the ArchUnit Contributor License Agreement.
Resolves #28

@codecholeric
Copy link
Collaborator

codecholeric commented Oct 20, 2017

Cool, thanks a lot 😃
I would not add the @Ignored test though, because the rest of the test (limiting the test to classes), is valid and useful right now. I would rather add the test without Java 8 specifics, but add a guard, that if we compile ArchUnit with Java 8 language level, the test will fail with a reminder, that the interface case should be added. Unfortunately something like

if (hasMethod(Collection.class, "stream")) {
    Assert.fail("Please extend the test case for accesses from interfaces " +
        "(which was not possible with Java 7)");
}

doesn't work, because even if we have source 1.7, we sometimes want to compile it with JDK 8, and then the test fails, even though it's not possible to use the language features. So maybe adding a TODO plus a guard in build.gradle

if (sourceCompatibility != JavaVersion.VERSION_1_7) {
    throw new IllegalStateException('Please fix TODOs marked with "TODO Java 8"')
}

would work (or do you have a better idea?)
Concerning your question, why the test case with the constant String doesn't work, the compiler inlines primitives and Strings, you can check this by

javap -v someInterface.class

and you'll see, that your String value is actually contained within the class file, thus on bytecode level, there is no access. You can fix this though, by making it an Object, or similar, because those won't be inlined (maybe we don't need the Java 8 guard then, either, because the test can just be written that way?)
Also: Do you think, it would make sense, to add an archunit-example for this added syntax, together with an archunit-integration-test?

@hajotka
Copy link
Author

hajotka commented Oct 29, 2017

Indeed, when changing the example to access an Object field, the tests work. So no need for a Java 8 guard here.

I'll have a look at adding an integration test.

@codecholeric
Copy link
Collaborator

Glad to hear, that it works without any shady workaround 😃
I'll wait for the integration test and then merge the PR (in case you notice some problem while writing the test)

@hajotka
Copy link
Author

hajotka commented Nov 3, 2017

I've added examples + integration tests, hope they aren't too artificial ;)

There was a problem with the build in archunit-examples, a dependency was missing, running

../gradlew clean build -P example

in archunit-examples failed with a ClassNotFoundException. How about running the examples as a smoke test in the CI?

@codecholeric
Copy link
Collaborator

Ah, that dependency I've just removed a little while ago ☹️
Yes, I haven't run -P example, yes it should be tested to compile (even though the tests fail by intention). Still, I don't think I want the dependency on configuration: 'tests' in the long run, since I'm pondering about creating a standalone example project (compare #30)

Anyway, the PR looks good, thank you so much 😃 I'll merge that now...

@codecholeric codecholeric merged commit 6805a57 into TNG:master Nov 4, 2017
@codecholeric
Copy link
Collaborator

I've added the excludeCategories only where necessary, so I could remove the dependency again...

codecholeric added a commit that referenced this pull request Feb 21, 2021
Adds syntax for interface predicates and conditions, e.g. classes().that().areInterfaces().should().beInterfaces()

Resolves #28
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