Adds package annotation for affected tests #220

lordofthejars · 2017-10-10T15:46:45Z

Short description of what this resolves:

Adds package annotation for affected tests

Changes proposed in this pull request:

Adds @Tests annotation to set which production classes are affected by annotated tests. Useful for black box testing.
Adds library to scan classes of given package. It is a really light library (no external dependencies), offers a lot of operations that we might use in the future. Also maybe it can be used as a substitution of Javassist.

Fixes #199

lordofthejars · 2017-10-10T15:49:22Z

api/pom.xml

+  </parent>
+  <modelVersion>4.0.0</modelVersion>
+
+  <artifactId>api</artifactId>


Created a new artifact so users of this annotation don't need to import all smart testing affected strategy jar but just an special artifact. Since I think that in the future we might use more annotations, @MatousJobanek and me decided this would be as start a good place to put it.

lordofthejars · 2017-10-10T15:50:06Z

pom.xml

@@ -47,6 +47,7 @@
    <version.assertj-core>3.8.0</version.assertj-core>
    <version.system-rules>1.16.1</version.system-rules>
    <version.javassist>3.21.0-GA</version.javassist>
+    <version.fast-classpath-scanner>2.7.4</version.fast-classpath-scanner>


Really light project for scanning classpath. I think we can use more and more features it offers.

bartoszmajsak

I have few remarks about the code base, but before we work on them to push it upstream I would like to ask a general question - how useful this feature is?

From the user perspective putting more effort to write tests (adding more constructs such as annotations) to let us run relevant tests does not really sound "smart as we advertise it" and I personally would be quite reluctant to use it. It also poses a question what if we make a mistake in this linkage or it will become obsolete? Do we have any way to detect that?

bartoszmajsak · 2017-10-18T06:39:43Z

...d/src/test/java/org/arquillian/smart/testing/strategies/affected/fakeproject/test/YTest.java

+import org.junit.Ignore;
+import org.junit.Test;
+
+// Test ignored because it is a test that is used to in tests and not to be run as real test by test runner


I know what you are trying to say in this sentence, but for a person not familiar with test-bed and how we tests our test selection this sentence would be rather confusing - can you make it more digestable? :)

You can also put it as a part of @Ignore.

bartoszmajsak · 2017-10-18T06:41:31Z

...c/test/java/org/arquillian/smart/testing/strategies/affected/ClassDependenciesGraphTest.java

@@ -141,6 +145,48 @@ public void should_detect_all_changes_transitive() {
    }

    @Test
+    public void should_detect_all_changes_adding_package_annotated_transitive() {


Can we have an ftest covering this new functionality?

Yes, but not done yet until all discussions happens

lordofthejars · 2017-10-18T14:03:16Z

Look I know it is not perfect solution, but it is a easy quick win solution for next simple case:

@RunWith(SpringRunner.class)
@SpringBootTest
public class MyTests {

    @Autowired
    private MyInterface interface;
}

What's happen if I change is the implementation of MyInterface? Then this test is not going to be executed because the implementation is not as import.

Of course you might say that coverage approach is the solution, but it has some disadvantages, and being the most important that the information of coverage might become outdated, so we need to recalculate it often, and this means (or might means) either running the whole suite or have a way to just update the coverage information.

Another problem is what's happen if we have tests that checks if the service is up and running in our OPenShift cluster. Then it means that 1) the deployment code must have an agent or something to get the coverage, and 2) we need to copy this data from a remote site to some local storage, and the problem is that maybe you don't have access to that machine within the cluster.

About outdated information using annotation approach might happen but it is quite more difficult because of two things, the first one is that you can use a regexp so you can always set the base package which is something unlike that is going to happen, but also in case it happens, if we detect that an expression doesn't hit anything, then we just write a warning so user can see it.

lordofthejars · 2017-10-26T14:30:46Z

Tasks to be done before this PR can be started to be reviewed_

Write Functional test (this cannot be done until we release the version) The reason is that we ned to be able to annotate tests in dogfood project with smart testing annotations. Now just testing manually so we need a release to add it as reference. Now I have tested manually and it works. The idea is release a ST with this feature and then provide an automatic test.
Update documentation with this option
Measure the time spent when classpath scanning is done. Scanning org.jboss.arquillian.config.* takes 80 ms which are like 25 classes loaded in this package and all its subpackages.

bartoszmajsak · 2017-10-31T23:02:52Z

Write Functional test (this cannot be done until we release the version) The reason is that we ned to be able to annotate tests in dogfood project with smart testing annotations. Now just testing manually so we need a release to add it as reference. Now I have tested manually and it works. The idea is release a ST with this feature and then provide an automatic test.

Not sure if that is really a problem. If you can test it manually can't you automate? Refer to latest snapshot instead of fixed version. This will work for the builds, as extension should be "installed" in the local repo prior to executing functional tests. Or am I missing something?

MatousJobanek · 2017-11-01T15:12:27Z

If you can test it manually can't you automate?

Yeah I agree with @bartoszmajsak
How I would do it:

create a tag in dogfooding repo with the api dependency and the annotations used in the tests
- the version element of the smart ST api dependency will contain some variable smart.testing.version that can be passed as a Maven build parameter / system property
in the functional test apply the commit (tag)
run the embedded maven build with the system property smart.testing.version set to the current snapshot version

lordofthejars · 2017-11-01T15:17:48Z

Great idea, thanks Matous, I'll do it tomorrow. El 1 nov. 2017 4:12 p. m., "Matous Jobanek" <[email protected]> escribió:

…

If you can test it manually can't you automate? Yeah I agree with @bartoszmajsak <https://github.com/bartoszmajsak> How I would do it: 1. create a tag in dogfooding repo with the api dependency and the annotations used in the tests - the version element of the smart ST api dependency will contain some variable smart.testing.version that can be passed as a Maven build parameter / system property 2. in the functional test apply the commit (tag) 3. run the embedded maven build with the system property smart.testing.version set to the current snapshot version — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#220 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABcmYSMWYLqDF0ZV7SLvrSLQ8RR_AiOWks5syIpcgaJpZM4P0KKE> .

bartoszmajsak · 2017-11-01T21:56:59Z

run the embedded maven build with the system property smart.testing.version set to the current snapshot version

Yeah, this would make dogfood repo buildable (in a sense of dependencies), so sure, that can work. It would be great to explain in the test itself why we do it though.

bartoszmajsak

Few minor things from my side to chew over. Overall nice addition! Thanks @lordofthejars

bartoszmajsak · 2017-11-01T22:00:03Z

api/src/main/java/org/arquillian/smart/testing/strategies/affected/Tests.java

+@Target(ElementType.TYPE)
+@Repeatable(TestsList.class)
+@Documented
+public @interface Tests {


By looking at this annotation my first thought was that it will execute list of test classes defined in it, so the name is a bit ambiguous. Maybe @Covers or @ComponentUnderTest makes it more self-explanatory?

bartoszmajsak · 2017-11-01T22:01:00Z

api/src/main/java/org/arquillian/smart/testing/strategies/affected/TestsList.java

+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.TYPE)
+@Documented
+public @interface TestsList {


Then this can become @ComponentsUnderTest

bartoszmajsak · 2017-11-01T22:01:55Z

...d/src/main/java/org/arquillian/smart/testing/strategies/affected/ClassDependenciesGraph.java

+
+        for (Tests tests : allTestsAnnotation) {
+            List<String> packages = getPackages(testJavaClass.packageName(), tests);
+            for (String packag : packages) {


Typo packag :)

Well it is not a type, package is a reserved word, so I decided to put in this way. But any other suggestion is welcome :)

That was late hahahaha. pkg maybe? :)

bartoszmajsak · 2017-11-01T22:03:59Z

...c/test/java/org/arquillian/smart/testing/strategies/affected/ClassDependenciesGraphTest.java

+
+        // when
+        Set<File> mainObjectsChanged = new HashSet<>();
+        mainObjectsChanged.add(new File(Unwanted.class.getResource("Unwanted.class").getPath()));


How about extracting this to a simple utility method asFile("Class.class")? Would make it a bit easier to follow, as we use it in few places.

Or maybe just this part Unwanted.class.getResource("Unwanted.class").getPath(). From the test itself it's not really important how we fetch it and it makes it a bit blurry to follow.

bartoszmajsak · 2017-11-01T22:05:19Z

...d/src/test/java/org/arquillian/smart/testing/strategies/affected/fakeproject/test/YTest.java

+import org.junit.Ignore;
+import org.junit.Test;
+
+// Test ignored because it is used internally


How about making this comment part of @Ignore?

bartoszmajsak · 2017-11-01T22:05:34Z

...d/src/test/java/org/arquillian/smart/testing/strategies/affected/fakeproject/test/ZTest.java

+import org.junit.Test;
+
+// Test ignored because it is used internally
+@Ignore


How about making this comment part of @Ignore?

bartoszmajsak · 2017-11-01T22:13:45Z

docs/configuration.adoc

@@ -138,6 +138,36 @@ IMPORTANT: This strategy is currently only applicable for _white box_ testing ap

 WARNING: At this moment, this strategy does not work with Java 9.

+===== Explicitly Set
+
+By default affected strategy uses _imports_ of tests to build the graph of dependencies.


Maybe graph of collaborators - dependency is rather broad term and could be understood that we build a graph of libraries we use based on imports

bartoszmajsak · 2017-11-01T22:14:32Z

docs/configuration.adoc

+This approach is fine for unit tests (white box tests) but might not work in all cases of high level tests (black box test).
+
+There are some test technologies that allows you to deploy an application locally and then run tests against it.
+For example Wildfly Swarm has `@DefaultDeployment` annotation or for example Spring (Boot) deploys all application automatically.


I would remove second for example in this statement. Also deploys entire application maybe?

bartoszmajsak · 2017-11-01T22:15:03Z

docs/configuration.adoc

+This means that production classes are not directly imported into the test, so there is no way to get a relationship between test and production classes.
+For this reason `affected` provides an annotation to set package(s) of production classes that are deployed by the test.
+
+The first thing you need to do is register next artifact into your build script: `org.arquillian.smart.testing:api:<version>`.


next artifact -> following artifact?

bartoszmajsak · 2017-11-01T22:15:32Z

docs/configuration.adoc

+
+The first thing you need to do is register next artifact into your build script: `org.arquillian.smart.testing:api:<version>`.
+
+Then you can annotate your test with `org.arquillian.smart.testing.strategies.affected.Tests` annotation.


Could be awesome to extract org.arquillian.smart.testing.strategies.affected.Tests automatically from the code - sounds like small doable extension?

I am not sure if small but yes, let me open a new issue, since we are using the same approach in several places and several files I think it is better to implement as a separate PR.

Yes, by all means, do so. I didn't mean to have it as part of this PR.

lordofthejars requested review from bartoszmajsak and MatousJobanek October 10, 2017 15:46

lordofthejars commented Oct 10, 2017

View reviewed changes

lordofthejars added Status: Work In Progress Status: Review Needed labels Oct 11, 2017

bartoszmajsak reviewed Oct 18, 2017

View reviewed changes

lordofthejars added 2 commits October 31, 2017 10:27

[WiP] Adds package annotation for affected tests

e818a53

Updates pom.xml

02554bb

lordofthejars force-pushed the issue-199 branch from 7637f52 to 02554bb Compare October 31, 2017 10:15

Minor changes and updates documentation

971cf8d

lordofthejars changed the title ~~[WiP] Adds package annotation for affected tests~~ Adds package annotation for affected tests Oct 31, 2017

lordofthejars removed the Status: Work In Progress label Oct 31, 2017

bartoszmajsak requested changes Nov 1, 2017

View reviewed changes

lordofthejars added 2 commits November 2, 2017 18:04

Code review changes and creation of ftest

34859ea

Resolves merge conflicts

074b51a

lordofthejars closed this Nov 2, 2017

lordofthejars deleted the issue-199 branch November 2, 2017 17:44

lordofthejars restored the issue-199 branch November 2, 2017 17:45

lordofthejars reopened this Nov 2, 2017

lordofthejars closed this Nov 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds package annotation for affected tests #220

Adds package annotation for affected tests #220

lordofthejars commented Oct 10, 2017

lordofthejars Oct 10, 2017

lordofthejars Oct 10, 2017

bartoszmajsak left a comment •

edited

Loading

bartoszmajsak Oct 18, 2017

bartoszmajsak Oct 18, 2017

lordofthejars Oct 18, 2017

lordofthejars commented Oct 18, 2017

lordofthejars commented Oct 26, 2017 •

edited

Loading

bartoszmajsak commented Oct 31, 2017

MatousJobanek commented Nov 1, 2017

lordofthejars commented Nov 1, 2017 via email

bartoszmajsak commented Nov 1, 2017

bartoszmajsak left a comment

bartoszmajsak Nov 1, 2017

bartoszmajsak Nov 1, 2017

bartoszmajsak Nov 1, 2017

lordofthejars Nov 2, 2017

bartoszmajsak Nov 2, 2017

bartoszmajsak Nov 1, 2017

bartoszmajsak Nov 1, 2017

bartoszmajsak Nov 1, 2017

bartoszmajsak Nov 1, 2017

bartoszmajsak Nov 1, 2017

bartoszmajsak Nov 1, 2017

bartoszmajsak Nov 1, 2017

bartoszmajsak Nov 1, 2017

lordofthejars Nov 2, 2017

bartoszmajsak Nov 2, 2017

lordofthejars Nov 2, 2017


		The first thing you need to do is register next artifact into your build script: `org.arquillian.smart.testing:api:<version>`.

		Then you can annotate your test with `org.arquillian.smart.testing.strategies.affected.Tests` annotation.

Adds package annotation for affected tests #220

Adds package annotation for affected tests #220

Conversation

lordofthejars commented Oct 10, 2017

Short description of what this resolves:

Changes proposed in this pull request:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoszmajsak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordofthejars commented Oct 18, 2017

lordofthejars commented Oct 26, 2017 • edited Loading

bartoszmajsak commented Oct 31, 2017

MatousJobanek commented Nov 1, 2017

lordofthejars commented Nov 1, 2017 via email

bartoszmajsak commented Nov 1, 2017

bartoszmajsak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoszmajsak left a comment •

edited

Loading

lordofthejars commented Oct 26, 2017 •

edited

Loading