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

Don't override tests annotations #433

Merged
merged 14 commits into from May 16, 2018

Conversation

@sbihel
Copy link
Contributor

sbihel commented May 14, 2018

No description provided.

sbihel added 2 commits May 14, 2018
@sbihel

This comment has been minimized.

Copy link
Contributor Author

sbihel commented May 14, 2018

I am not sure why the tests fail, I added a test that purposefully fail, but it's not the only one. Here are the 3 errors:

[ERROR] Failed to execute goal org.pitest:pitest-maven:1.3.0:mutationCoverage (default-cli) on project MockitoDemo: Execution default-cli of goal org.pitest:pitest-maven:1.3.0:mutationCoverage failed: No mutations found. This probably means there is an issue with either the supplied classpath or filters.
2:09:26 PM PIT >> SEVERE : Description [testClass=info.sanaulla.dal.BookDALTest, name=info.sanaulla.dal.BookDALTest] did not pass without mutation.
[ERROR] Failed to execute goal org.pitest:pitest-maven:1.3.0:mutationCoverage (default-cli) on project example: Execution default-cli of goal org.pitest:pitest-maven:1.3.0:mutationCoverage failed: The supplied filter would cause PIT to try and mutate itself.
@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented May 14, 2018

Hi @sbihel

I am not sure what are you trying to achieve here.

DSpot will surround the body of a test method with try/catch/fail block when the given test method fails.

However, if there is annotation @Test( expected = .... ) and a such exception is thrown, the test method doesn't fail, and thus DSpot doesn't surround the body with try/catch/fail, right?

@sbihel

This comment has been minimized.

Copy link
Contributor Author

sbihel commented May 14, 2018

I ran DSpot on jsoup, and for:

@Test(expected = IllegalArgumentException.class)
public void testThrowsOnPrependNullText() {
  Document doc = Jsoup.parse("<div id=1><p>Hello</p></div>");
  Element div = doc.getElementById("1");
  div.prependText(null);
}

it produced

@Test(timeout = 10000)
public void testThrowsOnPrependNullText_failAssert1() throws Exception {
    try {
        Document doc = Jsoup.parse("<div id=1><p>Hello</p></div>");
        Element div = Jsoup.parse("<div id=1><p>Hello</p></div>").getElementById("1");
        Jsoup.parse("<div id=1><p>Hello</p></div>").getElementById("1").prependText(null);
        org.junit.Assert.fail("testThrowsOnPrependNullText should have thrown 
 llegalArgumentException");
    } catch (IllegalArgumentException eee) {
    }
}

Is this an amplified test case or a temporary one?

@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented May 14, 2018

Hum, You probably found a bug.

This an amplified test case, but It should not be obtained.

The problem is in AmplificationHelper#prepareTestMethod.

I used setValues, and thus the existing values of the Annotation @Test are erased and replaced by just timeout = 10000.

A quick fix is to use the method addValue rather than setValues when there are some values in the annotation @Test (and replace the timeout value maybe...).

Can you propose a pull request with a test case that produce this behavior, and fix the bug?

Thank you very much, this is very good!

@sbihel sbihel changed the title Don't add try/catch for expected exceptions Don't override tests annotations May 14, 2018
sbihel added 7 commits May 14, 2018
This reverts commit 68ee3ac.
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 15, 2018

Pull Request Test Coverage Report for Build 1127

  • 28 of 28 (100.0%) changed or added relevant lines in 1 file are covered.
  • 54 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.3%) to 83.913%

Files with Coverage Reduction New Missed Lines %
dspot/src/main/java/fr/inria/stamp/Main.java 5 82.26%
dspot/src/main/java/fr/inria/stamp/Configuration.java 15 51.61%
dspot/src/main/java/fr/inria/stamp/diff/SelectorOnDiff.java 34 0.0%
Totals Coverage Status
Change from base Build 1111: -0.3%
Covered Lines: 3568
Relevant Lines: 4252

💛 - Coveralls
@sbihel

This comment has been minimized.

Copy link
Contributor Author

sbihel commented May 15, 2018

Sorry for all the commits. I couldn't directly modify the annotation, but it is ok as test annotations only have timeout and expected arguments.

@@ -367,9 +370,27 @@ public static void prepareTestMethod(CtMethod cloned_method, Factory factory) {
testAnnotation.setAnnotationType(ref);

Map<String, Object> elementValue = new HashMap<>();
elementValue.put("timeout", timeOutInMs);
java.lang.annotation.Annotation originalTestAnnotation = cloned_method.getAnnotation(org.junit.Test.class);

This comment has been minimized.

Copy link
@danglotb

danglotb May 15, 2018

Member

This might not work if the junit version of the program under amplification is different than the version that DSpot uses.
You can reuse:

 CtAnnotation testAnnotation = cloned_method.getAnnotations().stream()
                .filter(annotation -> annotation.toString().contains("Test"))
                .findFirst().orElse(null);

And works on Spoon nodes instead of the java.lang.annotation.Annotation.
Also, you don't have to re-add the expected values. You can just add the timeout value, using testAnnotation.addValue("timeout", factory.createLiteral(10000))
, right?

@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented May 15, 2018

Hi @sbihel,

Please, look at my comment. Also, I am avalaible on Riot to discuss about the solution.

Sorry for all the commits.

Don't be, I usually squash pr's commits into one.

sbihel added 3 commits May 15, 2018
if (valueOriginalTimeout < timeOutInMs) {
CtLiteral newTimeout = new CtLiteralImpl<Integer>();
newTimeout.setValue(timeOutInMs);
originalTimeout.replace(newTimeout);

This comment has been minimized.

Copy link
@danglotb

danglotb May 16, 2018

Member

Please, use the factory rather than the constructor:factory.createCtLiteral(timeOutInMs);
Also, I would prefer to use testAnnotation.addValue("timeout, factory.createCtLiteral(timeOutInMs)); instead of replacement of the literal inside the Annotation. In this way, we have some check done by Spoon on the values, and possibly capture bugs.

This comment has been minimized.

Copy link
@sbihel

sbihel May 16, 2018

Author Contributor

About the addValue, I tried that but the result was something like expected = 1000,10000. And there isn't a removeValue.

This comment has been minimized.

Copy link
@danglotb

danglotb May 16, 2018

Member

Ah okay, so I guess we will do like that, thank you very much.

sbihel added 2 commits May 16, 2018
@danglotb danglotb merged commit 15f7a50 into STAMP-project:master May 16, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.3%) to 83.913%
Details
@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented May 16, 2018

Thanks!

@sbihel sbihel deleted the sbihel:expected_failure branch May 16, 2018
@sbihel

This comment has been minimized.

Copy link
Contributor Author

sbihel commented May 16, 2018

🙌

Thanks for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.