-
Notifications
You must be signed in to change notification settings - Fork 966
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
Fix Optional#get() and string comparison bugs in JobService #804
Conversation
Clears a FIXME left from #650, get() uses that will always fail. Also fixes a bunch of == comparisons on strings, reference equality is not what you want there.
It makes trying to read output in GitHub Actions horrible. Seeing failures in red is kind of nice, so `--no-transfer-progress` keeps colors unlike `--batch-mode`.
/assign @zhilingc |
@Test(expected = NoSuchElementException.class) | ||
public void testStopJobForUnknownId() { | ||
var request = StopIngestionJobRequest.newBuilder().setId("bogusJobId").build(); | ||
jobService.stopJob(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't fail before the fix since it was the same exception type anyway. I don't think there's an easy way to assert on a thrown exception's message in JUnit 4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are potentially adding JUnit 5 dependency to our fork, if you would like to do it now and use assertThrows to validate the message.
JUnit 5 does work well with 4, so no need to upgrade all tests when adding it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Gojek folks tried/wanted to before—the Java SDK already uses it—but there was some reason it could not or should not be done? IIRC either @davidheryanto or @zhilingc mentioned it to me somewhere awhile back.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ches, pyalex The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
Thanks for fixing my bad code practices. 😥
This was pretty much on point 😮, this is my first stab at writing Java in a production system. How did you know?
Will check that out. |
What this PR does / why we need it:
Fixes some bugs in the Ingestion Job Management service.
Cleaning house a bit and I had a todo from here about the
get()
calls that will always fail. These don't actually have much impact sinceget()
already throwsNoSuchElementException
which we're doing anyway, but we'd lose the message we try to set.Actually perhaps more concerning is a bunch of
==
comparisons on strings, those might be a source of more functionally significant bugs for filtering in the list jobs API. I'm surprised that no tests trip something here—a little short on time to look more closely at the existing tests now, apologies.These issues are all flagged by IntelliJ. The
==
andthis.
receiver for all class member references smell like the author was writing mostly Python at the time 😉 I'm sympathetic to not always liking IDEs, but if you use a lighter editor I'd recommend setting up something like the SpotBugs Maven plugin, which would be nice to do anyway IMO.Does this PR introduce a user-facing change?: