Disables extension when test execution is skipped #102

hemanik · 2017-08-08T14:48:52Z

Changes proposed in this pull request:

Check for maven.test.skip, skipTests and skipITs property honored by Surefire and Failsafe plugins.
Ignore smart-testing-extension registration/configuration.
Tests to verify the change depending on the logger message.

Fixes #73

lordofthejars · 2017-08-08T15:22:55Z

core/src/main/java/org/arquillian/smart/testing/Configuration.java

@@ -45,6 +45,10 @@ public boolean isModeSet() {
        return this.mode != null;
    }

+    public boolean isSkipTests() {
+        return Boolean.valueOf(System.getProperty("maven.test.skip", "false"));


also you can set it by using skipTestsproperty. If it is set then using -DskipTests then tests are also skipped.

The thing to check here would be if -DskipTests does not imply maven.test.skip set implictly.

bartoszmajsak · 2017-08-08T16:56:50Z

Pending tasks:

Handle failsafe in the same way

bartoszmajsak · 2017-08-08T16:57:19Z

...ava/org/arquillian/smart/testing/ftest/configuration/DisabledSmartTestingFunctionalTest.java

+        final List<TestResult> actualTestResults = project
+            .build()
+                .options()
+                    //.withSystemProperties("maven.test.skip", "true")


What are we testing here?

bartoszmajsak · 2017-08-08T16:58:39Z

...xtension/src/main/java/org/arquillian/smart/testing/mvn/ext/SmartTestingMavenConfigurer.java

@@ -33,7 +33,7 @@ public void afterProjectsRead(MavenSession session) throws MavenExecutionExcepti

        configuration = Configuration.load();

-        if (configuration.isDisabled()) {
+        if (configuration.isDisabled() && configuration.isSkipTests()) {


isSkipTestsSet reflects better what is the use case here. It doesn't have to follow the artificial javabean spec.

MatousJobanek · 2017-08-10T10:27:20Z

...xtension/src/main/java/org/arquillian/smart/testing/mvn/ext/SmartTestingMavenConfigurer.java

@@ -28,17 +28,21 @@

    private Configuration configuration;

+    private MavenProjectConfigurator mavenProjectConfigurator;


It is slightly confusing - storing the MavenProjectConfigurator as a global variable just because of reading system properties. If you retrieve the information from system properties, then it can be used as a public static method from separated class. The other case would be storing a boolean flag as a information that the ST tool has been initialized (based on information that it has (not) been disabled or the test has (not) been skipped)

bartoszmajsak · 2017-08-14T18:39:02Z

...t/java/org/arquillian/smart/testing/ftest/configuration/SkipTestExecutionFunctionalTest.java

+        logCapturingStream = new ByteArrayOutputStream();
+        PrintStream newPrintStream = new PrintStream(logCapturingStream);
+        oldPrintStream = System.out;
+        System.setOut(newPrintStream);


BuiltProject from Embedded Maven we are using under the hood in ProjectBuilder exposes getMavenLog(). So I was thinking - maybe instead of replacing the output stream to ours for capturing how about making this log available through DSL? This would also let us run those tests not sequentially.

bartoszmajsak · 2017-08-16T19:56:03Z

Can you check why the build is failing?

hemanik · 2017-08-17T08:26:04Z

When using the build with projects, the related project dependencies are not resolved.
In order to test for instance config/impl-base project, we also need to include dependent projects - config/api, core/api, core/spi, core/impl-base.
So for the current scenario then we should either include these 5 projects for the test or the complete build.

bartoszmajsak · 2017-08-17T08:42:37Z

When using the build with projects, the related project dependencies are not resolved.
In order to test for instance config/impl-base project, we also need to include dependent projects - config/api, core/api, core/spi, core/impl-base.
So for the current scenario then we should either include these 5 projects for the test or the complete build.

This is how Maven works unfortunately.

Other option would be to take one of the most upper projects, like -api which does not have any dependency on the other modules from the project.

bartoszmajsak · 2017-08-18T08:35:51Z

...-bed/src/main/java/org/arquillian/smart/testing/ftest/testbed/project/BuildConfigurator.java

@@ -34,6 +34,8 @@
    private boolean mvnDebugOutput;
    private boolean enableSurefireRemoteDebugging = false;
    private boolean ignoreBuildFailure = false;
+    private boolean skipTests = false;
+    private String mavenLog = "false";


This is rather fuzzy. I don't really see a reason to have it as a string / container for the log content and a "boolean flag".

bartoszmajsak · 2017-08-18T08:48:02Z

...-bed/src/main/java/org/arquillian/smart/testing/ftest/testbed/project/BuildConfigurator.java

+        return this;
+    }
+
+    public BuildConfigurator withMavenLog() {


It's not really needed. Maven log is always kept in the case of how we use embedded maven, so simple method for retrieving it from the build is enough. This is making it overcomplicated considering how it's done underneath.

…xecution is skipped.

…ogs for extension verification.

… part of build.

MatousJobanek · 2017-08-18T12:35:03Z

mvn-extension/src/main/java/org/arquillian/smart/testing/mvn/ext/MavenProjectConfigurator.java

+    }
+
+    private boolean isSkip() {
+        return Boolean.valueOf(System.getProperty("maven.test.skip", "false"));


Please, move these methods outside of this class. The logic retrieves static information - so, they can be static methods.
See this comment: #102 (comment)

MatousJobanek · 2017-08-18T12:35:47Z

...xtension/src/main/java/org/arquillian/smart/testing/mvn/ext/SmartTestingMavenConfigurer.java

    @Override
    public void afterProjectsRead(MavenSession session) throws MavenExecutionException {

        configuration = Configuration.load();

-        if (configuration.isDisabled()) {
+        final MavenProjectConfigurator mavenProjectConfigurator = new MavenProjectConfigurator(configuration);


As I have written above - it is not necessary to create an instance of the class just because of retrieving system properties...

…ality to another class.

hemanik added Status: Work In Progress Status: Review Needed labels Aug 8, 2017

hemanik requested a review from bartoszmajsak August 8, 2017 14:49

lordofthejars requested changes Aug 8, 2017

View reviewed changes

bartoszmajsak requested changes Aug 8, 2017

View reviewed changes

bartoszmajsak reviewed Aug 8, 2017

View reviewed changes

hemanik force-pushed the disable-extension branch 3 times, most recently from 764040a to 0b4dc9d Compare August 10, 2017 07:19

MatousJobanek reviewed Aug 10, 2017

View reviewed changes

hemanik force-pushed the disable-extension branch 5 times, most recently from f4516e1 to a83c3dc Compare August 14, 2017 18:13

bartoszmajsak reviewed Aug 14, 2017

View reviewed changes

hemanik force-pushed the disable-extension branch 2 times, most recently from adde2c0 to f827ef2 Compare August 16, 2017 17:37

hemanik force-pushed the disable-extension branch from f827ef2 to 1d559eb Compare August 17, 2017 06:57

hemanik removed the Status: Work In Progress label Aug 17, 2017

hemanik force-pushed the disable-extension branch from b847882 to e662b9b Compare August 17, 2017 09:06

bartoszmajsak removed the Status: Review Needed label Aug 17, 2017

bartoszmajsak requested changes Aug 18, 2017

View reviewed changes

hemanik added 3 commits August 18, 2017 16:17

feat(disable-smart-testing-extension): Disables extension when test e…

1139375

…xecution is skipped.

feat(disable-smart-testing-extension): updates DSL to include maven l…

b47fc98

…ogs for extension verification.

fix(travis-build): Unresolved dependencies when sub-projects given as…

b4b671e

… part of build.

hemanik force-pushed the disable-extension branch from e662b9b to b4b671e Compare August 18, 2017 11:46

MatousJobanek reviewed Aug 18, 2017

View reviewed changes

chore(maven-project-configurator): refactors to extract skip function…

2c02450

…ality to another class.

hemanik force-pushed the disable-extension branch from d1a273a to 2c02450 Compare August 21, 2017 11:16

MatousJobanek approved these changes Aug 21, 2017

View reviewed changes

bartoszmajsak approved these changes Aug 21, 2017

View reviewed changes

bartoszmajsak merged commit b26a69a into arquillian:master Aug 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disables extension when test execution is skipped #102

Disables extension when test execution is skipped #102

hemanik commented Aug 8, 2017 •

edited

lordofthejars Aug 8, 2017

bartoszmajsak Aug 8, 2017

bartoszmajsak commented Aug 8, 2017 •

edited by hemanik

bartoszmajsak Aug 8, 2017

bartoszmajsak Aug 8, 2017

MatousJobanek Aug 10, 2017

bartoszmajsak Aug 14, 2017

bartoszmajsak commented Aug 16, 2017

hemanik commented Aug 17, 2017

bartoszmajsak commented Aug 17, 2017

bartoszmajsak Aug 18, 2017

bartoszmajsak Aug 18, 2017 •

edited

hemanik Aug 18, 2017

MatousJobanek Aug 18, 2017

MatousJobanek Aug 18, 2017

		@@ -28,17 +28,21 @@

		private Configuration configuration;

		private MavenProjectConfigurator mavenProjectConfigurator;

Disables extension when test execution is skipped #102

Disables extension when test execution is skipped #102

Conversation

hemanik commented Aug 8, 2017 • edited

Changes proposed in this pull request:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoszmajsak commented Aug 8, 2017 • edited by hemanik

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 commented Aug 16, 2017

hemanik commented Aug 17, 2017

bartoszmajsak commented Aug 17, 2017

Choose a reason for hiding this comment

bartoszmajsak Aug 18, 2017 • edited

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

hemanik commented Aug 8, 2017 •

edited

bartoszmajsak commented Aug 8, 2017 •

edited by hemanik

bartoszmajsak Aug 18, 2017 •

edited