A change was submitted for review: https://review.gerrithub.io/c/jenkinsci/gerrit-code-review-plugin/+/513811
The problem stems from Timestamp subclassing Date, which attempts to normalize the representation into local timezone. We're also using Gson's built-in Date/Timestamp type adapter, which also assumes that it should serialize date objects into local timezone. There is no way to change that behavior in Gson directly, so the solution is to register our own Gerrit-compatible TypeAdapter that serializes the Timestamp object to Gerrit-compatible format (like ISO-8601, but no 'T', no 'Z', and no timezone offset), in UTC timezone.
The unit test was the hardest part about this: the current tests don't do anything to validate the format that actually gets sent over the wire, and they only set some fields but not others. Using the MockServerClient, there are two approaches we could take when stubbing the endpoints with getClient().when():
- Register the URL, method, and/or full expected request body as the stub
- This means if anything was unexpected in one of those, the test fails with "404" instead of explaining what was missing or unexpected
- Register just the URL, or URL+method, but don't set expectations on the body; and instead validate the body using getClient().verify()
- This way, if a request matches the URL and method, but the body was unexpected, the test failure explains the expected/actual differences, making it easier to identify what went wrong.
However, I wasn't able to get JsonBody to work with verify() #2; and since we aren't mocking the system clock, I also couldn't set the expected request body with #1 because I couldn't predict System.currentTimeMillis(). Instead, I added a way to extract the full request body from the MockServerClient, parse it as JSON, and extract the field I wanted to check. Then because of the unpredictable time, I could only verify the format via simple regex, but then I parse that string back to a long (epoch seconds) and compare that to the current time. Before the fix, the parsed time would be Z-hours off from current time (where Z = timezone offset).
Unfortunately, that means the test could still produce a false-pass, if it is executed on a JVM that is set to UTC time (i.e., my change from Patchset 1 which contains only the test). This could be fixed by setting the timezone before starting the test, but I was trying not to spend too much time changing how the whole jenkins environment is configured, and I'm also not entirely sure the "best way" to set the Jenkins timezone in a unit test.
If I were going to make more changes here, I would:
- Add the ability to inject the system clock used at GerritCheckStep.setCheckTimestamps, so that we can mock it in the unit tests, and verify the exact JSON timestamp output
- Configure/mock the timezone that will be inferred when JenkinsRule starts up, so we can verify that a non-UTC server will send timestamps in UTC.
- Maybe a better way to use the mock API endpoint, so that request inputs are easier to verify.
- Consider moving away from java.sql.Timestamp (both here and in the gerrit-checks plugin source), and use something easier to work with; like maybe Instant, ZonedDateTime, or Calendar.