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

First attempt for api test #2919

Closed
wants to merge 1 commit into from

Conversation

GabrielXia
Copy link
Contributor

@GabrielXia GabrielXia commented Apr 27, 2017

Contains

A first attempt to #2317. In general, with @Cervator and @iaronaraujo, we will try to detect all the api method changes in a PR and detect the correct scroll to increment.

In this PR, I wrote a first test for org.terasology.engine.ComponentFieldUri. It'll do:

  1. Test all public constructors and methods, if there is a change in their signatures (modifiers, parameters, return type and exceptions) or a remove of a method , majorIncreaseTest() print "Major increase required" with details
  2. If the class has a new public method & constructor, minorIncreaseTest() will print "Minor increase requierd"
  3. There are also other tests generated by Evosuite for code coverage. If they are not passed a major increment might be needed
  4. If all the api-tests pass, but the test in regular tests in engine-tests don't 100% pass, that might be a minor increment
  5. If all the tests pass, then that's a patch increment

In most cases, tests generated by Evosuite will detect method signature changes, too. E.g. If we change a parameter in a method, some tests will not be complied. Even if these tests don't detect the method changes, majorIncreaseTest() will detect it.

How to test

In org.terasology.engine.ComponentFieldUri, try to do some changes in the method and run the tests

  • If you change the the parameters or return types of a public method or remove a method, normally the tests can't be compiled. If you modified the modifiers, e.g. public to public final, majorIncreaseTest() will detect it
  • If you add a new public method & constructor, minorIncreaseTest() will detect it

Outstanding before merging

  • Need to consider the workflow of the API test
  • Need to evaluate the MethodSignature detection
  • Need to evaluate whether the tests generated by Evosuite has meanings.

Future work

  • Add more meaningful tests
  • Tests to all API classes
  • A scraper to show what API we have covered by the API test

Do I miss something? Thanks for comments :-)

@GooeyHub
Copy link
Member

Hooray Jenkins reported success with all tests good!

@@ -49,5 +49,6 @@ dependencies {
compile group: 'junit', name: 'junit', version: '4.12'
compile group: 'org.mockito', name: 'mockito-all', version: '1.10.19'
compile group: 'org.jboss.shrinkwrap', name: 'shrinkwrap-depchain-java7', version: '1.1.3'
compile group: 'org.evosuite', name:'evosuite-standalone-runtime', version: '1.0.4'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if "evosuite-standalone-runtime" is necessary. In this test, only verifyException() method is used from the library. Maybe we can just remove these tests with verifyException() if we don't want use evosuite library

}

@Test(timeout = 4000)
public void constructorTest1() throws Throwable {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cervator If in a pr someone changes the code in a method, then the method returns a different value. In this case, some auto-generated test will fail. It needs a major increase or a minor increase? IMHO it's a major increase.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tricky situation. Based solely on SemVer I don't think its spec cares one way or another: so long as the signatures match you pass SemVer. Logic changes are assumed to be benevolent (working toward fixing bugs or providing new features). A failed unit test either means you introduced a bug or failed to update a unit test, which should block a PR for a reason unrelated to SemVer.

So: broken unit test = fix your code! No merge for you until then :-) SemVer doesn't really matter until the PR qualifies as merge-able.

@Cervator Cervator added the Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. label Sep 9, 2017
@Cervator
Copy link
Member

Cervator commented Sep 9, 2017

Just to make sure I understand the current state of the approach here: the example test prepares a mockup of the target class dynamically, then validates that it still matches public methods etc right? Wouldn't we want to load in a stored set of data from a prior build? Or is that more or less the next step and some of this stuff makes it easy to switch over to do so? :-)

@GooeyHub
Copy link
Member

GooeyHub commented Sep 9, 2017

Hooray Jenkins reported success with all tests good!

@oniatus
Copy link
Contributor

oniatus commented Jan 21, 2018

Interesting approach 👍 I think this is feature-complete as experiment so probably it is okay to close it @GabrielXia? I would like to finalize the discussion and strategy about API testing on #2317 and see what we can come up with. There is one different approach we could try out before selecting one way to go.

@Cervator
Copy link
Member

Bump - #3253 has now been merged, which uses the other approach that may be simpler than this.

@GabrielXia can you play around with the other system and compare it to your own here to see if any further improvements can be made? There are also some follow-up issues.

If this is complete and remaining an experiment it would be nice to close it, I just want to make sure we don't lose anything nice and useful. And in either case the work is hugely appreciated :-)

@syntaxi
Copy link

syntaxi commented May 22, 2018

I'm going to close this as superseded by #3253. Thanks for the experimental work 👍

@syntaxi syntaxi closed this May 22, 2018
@Cervator Cervator added the Status: Needs Investigation Requires to be debugged or checked for feasibility, etc. label May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Status: Needs Investigation Requires to be debugged or checked for feasibility, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants