{"action":"created","issue":{"url":"https://api.github.com/repos/feedzai/feedzai-openml/issues/33","repository_url":"https://api.github.com/repos/feedzai/feedzai-openml","labels_url":"https://api.github.com/repos/feedzai/feedzai-openml/issues/33/labels{/name}","comments_url":"https://api.github.com/repos/feedzai/feedzai-openml/issues/33/comments","events_url":"https://api.github.com/repos/feedzai/feedzai-openml/issues/33/events","html_url":"https://github.com/feedzai/feedzai-openml/pull/33","id":345560578,"node_id":"MDExOlB1bGxSZXF1ZXN0MjA0NjYwMDc1","number":33,"title":"Add pitest mutation test plugin","user":{"login":"pedrorijo91","id":1999050,"node_id":"MDQ6VXNlcjE5OTkwNTA=","avatar_url":"https://avatars2.githubusercontent.com/u/1999050?v=4","gravatar_id":"","url":"https://api.github.com/users/pedrorijo91","html_url":"https://github.com/pedrorijo91","followers_url":"https://api.github.com/users/pedrorijo91/followers","following_url":"https://api.github.com/users/pedrorijo91/following{/other_user}","gists_url":"https://api.github.com/users/pedrorijo91/gists{/gist_id}","starred_url":"https://api.github.com/users/pedrorijo91/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pedrorijo91/subscriptions","organizations_url":"https://api.github.com/users/pedrorijo91/orgs","repos_url":"https://api.github.com/users/pedrorijo91/repos","events_url":"https://api.github.com/users/pedrorijo91/events{/privacy}","received_events_url":"https://api.github.com/users/pedrorijo91/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2018-07-29T20:32:34Z","updated_at":"2018-07-30T20:35:42Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/feedzai/feedzai-openml/pulls/33","html_url":"https://github.com/feedzai/feedzai-openml/pull/33","diff_url":"https://github.com/feedzai/feedzai-openml/pull/33.diff","patch_url":"https://github.com/feedzai/feedzai-openml/pull/33.patch"},"body":"This is an WIP experiment, which consists in adding the [pitest mutation testing framework](http://pitest.org/) into the codebase.\r\n\r\n# What is mutation testing?\r\n\r\nMutation testing may work as a better technique for validating the effectiveness of the existing tests. \r\n\r\nOur current approach (and the industry standard) has been to use LoC coverage (a few use alternative metrics like function coverage, branch coverage, statement coverage, and etc - more on the subject at https://blog.codacy.com/what-your-mother-didnt-tell-you-about-code-coverage-605253143936#9699). Nevertheless, (LoC) code coverage only measures how many lines of code have been executed during the test execution. This means it's possible to have 100% LoC coverage, and the tests may be \"weak\" as it may not assert the right properties (or the developer may even forget to assert something on the test). On the other hand, mutation testing changes (mutates) the source code and checks if the tests capture the behaviour change. A classic example is changing some `if` statement by reverting its condition:\r\n\r\n```\r\npublic long factorial(int n) {\r\n  if (n <= 1) {\r\n    return 1;   \r\n  } else {    \r\n    return(n * factorial(n-1));  \r\n  }\r\n}\r\n```\r\n\r\ngets mutated into\r\n\r\n```\r\npublic long factorial(int n) {\r\n  if (n > 1) { // note the condition was mutated\r\n    return 1;   \r\n  } else {    \r\n    return(n * factorial(n-1));  \r\n  }\r\n}\r\n```\r\nIf no test fails after this change, then there's something to improve on the tests. By introducing this framework we hope to improve our tests and decrease the possible bugs.\r\n\r\nNote that by its definition itself, mutation testing may be slow since it implies run the test suite after introducing each mutation (or group of mutations)\r\n\r\n# PR structure\r\n\r\nThis PR is still WIP, and aims to bring the following changes:\r\n\r\n- [x] Add pitest plugin\r\n- [x] Add pitest phase to CI\r\n- [ ] Use framework report to improve the codebase \r\n- [ ] Add documentation on README for developers wanting to contribute\r\n\r\n# Did pitest found any code problem already?\r\n\r\nWell, if it hadn't, this PR wouldn't exist :)\r\n\r\nThe first finding of pitest was that we have somehow redundant verifications on our codebase: https://github.com/feedzai/feedzai-openml/blob/master/openml-api/src/main/java/com/feedzai/openml/data/schema/DatasetSchema.java#L87-L95\r\n\r\nWhile the lines 87-90 only check the indexes are sorted, the lines 92-95 implicitly check the indexes are sorted because it compares the list of indexes with the range `0 until indexes.size()`, a comparison that would fail if the indexes weren't sorted.\r\n\r\nAnother example caught: NumericFieldTypeTest is not testing boundary values at https://github.com/feedzai/feedzai-openml/blob/master/openml-api/src/test/java/com/feedzai/openml/provider/fieldtype/NumericFieldTypeTest.java#L65-L70\r\n\r\nby mutating `if (parsedValue < this.minValue)` into `if (parsedValue <= this.minValue)` at https://github.com/feedzai/feedzai-openml/blob/master/openml-api/src/main/java/com/feedzai/openml/provider/descriptor/fieldtype/NumericFieldType.java#L164 , the tests still pass. Why are not ensuring the `minValue` is allowed on our tests? :)\r\n\r\nReally sure more dangerous cases will appear in the report (hadn't the time to look at everything since some cases are still being reported even if they aren't very relevant - see `failing builds` topic bello)\r\n\r\n# Implementation details\r\n\r\n1. failing builds\r\n\r\nIt's possible to introduce a threshold for which the build phase would fail (useful for CI): http://pitest.org/quickstart/maven/#mutationthreshold\r\n\r\nI would like to try 100% mutation testing coverage, but I'm not sure if possible, at least for now. I've seen a few reports complaining on `hashcode`/`equals`/`toString` methods not being properly tested, but I don't seem much value on that.\r\n\r\nThere's also a configuration to disable specific methods and classes that I need to try: \r\n\r\n- http://pitest.org/quickstart/maven/#excludedmethods \r\n- http://pitest.org/quickstart/maven/#excludedclasses\r\n- http://pitest.org/quickstart/maven/#avoidcallsto\r\n\r\n:warning: Still needs work (currently at 50% threshold) \r\n\r\n2. selected mutations\r\n\r\nI'm starting by enabling [all mutations](http://pitest.org/quickstart/maven/#mutators), but may decrease the list of mutations if we get some irrelevant results.\r\n\r\n3. multi module projects\r\n\r\nThe pitest framework has a problem dealing with multi module maven projects.\r\nThe solution [presented](http://pitest.org/quickstart/maven/#handling-projects-composed-of-mutiple-maven-modules-pitmp) is to rely on the [pitmp-maven-plugin](https://github.com/STAMP-project/pitmp-maven-plugin).\r\n\r\nTo execute the mutation test just use the following command on the project root (it's also possible to run in one of the submodules root):\r\n\r\n`mvn org.pitest:pitest-maven:mutationCoverage`\r\n\r\nand check the report on the `target` folder (inside a `pit-reports` folder)\r\n\r\n4. run mutation testing over the last commit\r\n\r\nThere's a command to run the framework on recent changes, which will probably be used to run on CI, but I still need to have a better look: http://pitest.org/quickstart/maven/#scmmutationcoverage-goal\r\n\r\n**Edit:** the referred configuration it seems to be an option to run mutation testing before commit/push, and not to run over a PR on CI, so it is not very useful for now\r\n\r\n5. parallelism \r\n\r\nIt is possible to configure the number of threads to use when mutation testing using the [threads configuration](http://pitest.org/quickstart/maven/#threads). By default uses a single thread.\r\n\r\nIn a very rough experiment on my mac (we can't even call it benchmark):\r\n\r\n* 1 thread takes 42s\r\n* 2 threads take 34s\r\n* 4 threads take 34s\r\n* 8 threads take 34s\r\n\r\n6. incremental analysis\r\n\r\nFor very large codebases there's an *experimental* feature on pitest: [incremental analysis](http://pitest.org/quickstart/incremental_analysis/) which optimises the runs by tracking changes to the code and tests and storing the results from previous runs. But it doesn't seem a very safe option for now. Will only have a look at it if the mutation testing phase starts taking too long"},"comment":{"url":"https://api.github.com/repos/feedzai/feedzai-openml/issues/comments/409001138","html_url":"https://github.com/feedzai/feedzai-openml/pull/33#issuecomment-409001138","issue_url":"https://api.github.com/repos/feedzai/feedzai-openml/issues/33","id":409001138,"node_id":"MDEyOklzc3VlQ29tbWVudDQwOTAwMTEzOA==","user":{"login":"TravisBuddy","id":33237381,"node_id":"MDQ6VXNlcjMzMjM3Mzgx","avatar_url":"https://avatars0.githubusercontent.com/u/33237381?v=4","gravatar_id":"","url":"https://api.github.com/users/TravisBuddy","html_url":"https://github.com/TravisBuddy","followers_url":"https://api.github.com/users/TravisBuddy/followers","following_url":"https://api.github.com/users/TravisBuddy/following{/other_user}","gists_url":"https://api.github.com/users/TravisBuddy/gists{/gist_id}","starred_url":"https://api.github.com/users/TravisBuddy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/TravisBuddy/subscriptions","organizations_url":"https://api.github.com/users/TravisBuddy/orgs","repos_url":"https://api.github.com/users/TravisBuddy/repos","events_url":"https://api.github.com/users/TravisBuddy/events{/privacy}","received_events_url":"https://api.github.com/users/TravisBuddy/received_events","type":"User","site_admin":false},"created_at":"2018-07-30T20:35:42Z","updated_at":"2018-07-30T20:35:42Z","author_association":"NONE","body":"Hey @pedrorijo91,  \nYour changes look good to me!\n"}}
{"actor":{"display_login":"TravisBuddy"}}