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

Unspecify assertions #413

Merged
merged 3 commits into from May 14, 2018

Conversation

Projects
None yet
3 participants
@sbihel
Copy link
Contributor

sbihel commented Apr 26, 2018

No description provided.

sbihel added some commits Apr 26, 2018

@sbihel

This comment has been minimized.

Copy link
Contributor Author

sbihel commented Apr 26, 2018

I am confused, on my machine I need to remove the comments for the tests to pass

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 26, 2018

Pull Request Test Coverage Report for Build 1068

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.7%) to 84.265%

Totals Coverage Status
Change from base Build 1057: -0.7%
Covered Lines: 3529
Relevant Lines: 4188

💛 - Coveralls
@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented Apr 26, 2018

Hi @sbihel

I think the problem come from the order of the execution of tests, which is very bad.

In your machine, you have a specific order, and on travis there is another one. So the boolean withComment is probably set to true by another one, executed before of travis, but after on your machine. I'm gonna look for this.

Anyway, thank you for the PR, it seems very good!

@sbihel

This comment has been minimized.

Copy link
Contributor Author

sbihel commented Apr 26, 2018

Ah I see. Always the same problem 😅

The only issue I see with using Sets, is that the error messages are ugly when the assertion fails

@sbihel

This comment has been minimized.

Copy link
Contributor Author

sbihel commented Apr 26, 2018

Should I refactor all similar tests also?

@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented Apr 26, 2018

@sbihel If you run only the test class fr.inria.diversify.dspot.amplifier.StatementAddTest, did it pass or not?

@sbihel

This comment has been minimized.

Copy link
Contributor Author

sbihel commented Apr 27, 2018

@danglotb no problem with only StatementAddTest or StatementAddTest,AssertGeneratorHelperTest or AssertGeneratorHelperTest,StatementAddTest but it fails with StatementAddTest,MethodsAssertGeneratorTest and MethodsAssertGeneratorTest,StatementAddTest.

The order given to -Dtest= isn't taken into account?

@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented Apr 28, 2018

The order given to -Dtest= isn't taken into account?

I don't know about that, but anyway, all the tests should pass, in any order there are executed.

I am confused, on my machine I need to remove the comments for the tests to pass

This is also confusing me. I was wrong when I talked about the DSpotUtils#withComment boolean. In any case, the comments should be attached to the added statements / generated assertions. This boolean only the printing of the comments, not the adding in the spoon model.

I don't understand what is going on. Could you please, provide me a new log, with your changes that should makes some tests pass, from the maven execution of the test, i.e. mvn test ?

@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented May 13, 2018

@sbihel Can we merge this PR?

@sbihel

This comment has been minimized.

Copy link
Contributor Author

sbihel commented May 14, 2018

@danglotb I guess so. There might be similar tests that could use this refactoring but we can change them later on.

@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented May 14, 2018

Hi @sbihel.

There might be similar tests that could use this refactoring but we can change them later on.

You are probably right, but I will merge this PR first, and then we will take care of the others.

@danglotb danglotb merged commit d179df1 into STAMP-project:master May 14, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.7%) to 84.265%
Details
@danglotb

This comment has been minimized.

Copy link
Member

danglotb commented May 14, 2018

Moreover, it seems that #432 is failing because of this. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment