Changed exception on hasBreakingChange to VerificationException #89
Conversation
| reportLink = null; | ||
| } | ||
| StringBuilder message = new StringBuilder("Detected binary changes.\n") | ||
| StringBuilder message = new StringBuilder("Verification failed: Detected binary changes.\n") |
There was a problem hiding this comment.
Do we have to add this string? Looks that the verification is already encoded in the exception type.
There was a problem hiding this comment.
No, it shouldn't be strictly necessary, but it does make it more explicit.
There was a problem hiding this comment.
Yeah I'm a bit worried though, because we can have scripts which search in the logs for that particular line.
There was a problem hiding this comment.
Removed and verified it's still classified as a Verification failure 👍
There was a problem hiding this comment.
Hi @melix, I saw tests are failing on Gradle <7.4 - VerificationException is only available from Gradle 7.4+, so to properly support down to Gradle 6.6 (your current min supported version), I had to change back to GradleException and modify the exception message - that is done in commit 2da8088. I'm very flexible about the wording of the modification, as long as it's something from this list 😄
I did try to search for any other mention of Detected binary changes in the code to fix any tests relying on that message, but I couldn't find anything. Could you point me to those scripts you mentioned so I can verify they still work and fix them if necessary?
There was a problem hiding this comment.
These are internal tools, not public.
ribafish
force-pushed
the
gk/fixVerificationFailure
branch
from
4f3ea34 to
e57f1b5
Compare
…exception message to support Gradle 6.6-7.4
|
Hi @melix, I had thought about this a bit more, and I believe there are only two ways to handle this:
Unfortunately, I don't see a different fix for this, and we'd recommend going with the first option, as it covers all Gradle versions. I am happy to address anything that breaks because of the changed exception message if necessary. |
|
Ok let's proceed with 1. Hopefully our internal tooling won't break :) |
This change is required for failures coming from the
japicmp-gradle-pluginto be correctly classified as verification failures when they happen due to detected binary changes.