Japicmp implementation in pinot-spi #15684
Conversation
…fChecker, added functionality for displaying line number, and reverted temporary change to TableConfig.java
…g sh file so that "No incorrect..." message only displayed once, and adding blank line check to GitDiffChecker
… work with pull requests.
This reverts commit 270937f.
…onditional to reflect return type change. java: switch from returning line number to string of code, as previous logic did not work if multiple "chunks" of code were changed, and added annotation logic that excluded json-related annotations.
…message and put text file in my module. java: slightly altered regex after rethinking it, removed System.out.println, i think it's not necessary
…ng semicolons and not curly braces
…turns nothing, there isn't an accidental test passing
…ecking. Switched to the japicmp plugin, with some modifications to the compatibility of checks that japicmp performs. With this, contributors will be able to see if they made incompatible SPI changes when running mvn clean, rather than waiting until they make a PR. Adding a .jar of pinot-spi that japicmp will use for comparisons.
UOETianleZhang
left a comment
UOETianleZhang
left a comment
There was a problem hiding this comment.
Thanks for making the change!
…justification for using a baseline jar for comparison, and that we need to eventually transition away from it.
|
For the jar file to compare, can we download it as part of the CI or use the previous release version? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #15684 +/- ##
============================================
- Coverage 62.90% 62.65% -0.25%
Complexity 1386 1386
============================================
Files 2867 2872 +5
Lines 163354 164004 +650
Branches 24952 25097 +145
============================================
+ Hits 102755 102760 +5
- Misses 52847 53510 +663
+ Partials 7752 7734 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Hi @xiangfu0, could you elaborate a little bit more on the idea of downloading as CI?
This is also what @matvj250 and I wanted to achieve. In theory, japicmp should compare the current commit with the last stable version (1.3.0 in this case) to make sure no back-incompatible interface changes. However, this is not possible because there are already back-incompatible changes in 1.4.0-SNAPSHOT. This is why we have the jar of the current commit as the baseline (temporarily). After 1.4.0 is released, we can delete the jar and update the base version to 1.4.0 in the plugin config. |
…ed pom file so that all pinot-spi files are checked
UOETianleZhang
left a comment
UOETianleZhang
left a comment
There was a problem hiding this comment.
LGTM. Thanks for making the change!
5 participants
Implementing japicmp in the pinot-spi module to fail builds (i.e. running mvn clean) when incompatible SPI changes are made, such as removing methods, changing method return types, etc. Compatible SPI changes, such as adding new methods, will not fail builds. I have tested that this works as intended.
Some additional information about the package:
Every commit except the last is irrelevant to japicmp, and relates to an old implementation I was working on for this. None of that old code is reflected in this pull request.