-
Notifications
You must be signed in to change notification settings - Fork 83
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
refactor: refactor the NPEFix pipeline #1172
Conversation
andre15silva
commented
Jan 18, 2021
•
edited
Loading
edited
- Moves logic to pipeline
- Introduces abstract NPEFix step
- Introduces MavenInspector
Needs to be rebased after #1171 is merged. |
5486613
to
5c389e5
Compare
Ready for review. Note, had to change Jenkins config because the junit command wouldn't run if previous stages failed. |
Hi Andre, Nice. We need to further split it into atomic PRs. The advantage of baby PRs is that 1) it easier to explain, understand and review 2) it enables us to write and run tests in isolation in CI. 3) since we squash PRs, we have a nicer and finer-grain commit history. By doing baby PRs, you'll become a great developer and master branch-by-abstraction. Here is a proposal of baby PRs from this one:
Plus, when you change an assertion, add a comment to explain why there is a change WDYT? |
Sounds very good. The order needs to be different tho, because there are some dependencies.
I'll open these PRs today. |
Perfect!
|
1228364
to
8792fb5
Compare
This will be ready after #1183 |
we're making progress |
8792fb5
to
5ec104b
Compare
Should be ready for review after CI. Note: the parameters for NPEFix initialization in the |
Overall this refactoring looks really good. Could you elaborate on the meaning of each new configuration option? We can have a few sentences, maybe a paragraph. Thanks! |
Introduces 4 new config options for NPERepair:
|
Now, there is a problem with this. Seems like I misunderstood what the option To include all the functionality described above for this option, I have add functionality to I propose #1185, which would enable this. |
7c661b6
to
d54f28b
Compare
Sorald's version that was on the repository is no longer available. CI is failing because of that. I'm adapting Apart from that it's ready. |
great. ping me back when it's done. |
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
Signed-off-by: André Silva <andre15andre@hotmail.com>
d54f28b
to
a79d420
Compare
Signed-off-by: André Silva <andre15andre@hotmail.com>
5284e63
to
48d705a
Compare
I'm having some issues with having My adaptation is here https://github.com/andre15silva/failingProject/tree/npefix-scope, and it has test cases that could be fixed by changing those files. I traced this option back to Spirals-Team/maven-repair@44f2a2d. @tdurieux, would it be possible for you to briefly explain/show an example where this would happen? Thank you! |
It could happen with silent NPE (catched NPE that NPEfix still try to fix) also the textual patch generation of NPEFix is not the most robust. |
Issue opened. Ready for review. |
Thanks a lot @andre15silva ! That's a big milestone, and we can now continue with Java 11 support. |