Skip to content

Conversation

@NHDaly
Copy link
Member

@NHDaly NHDaly commented Jan 14, 2023

Correctly account for the time sent from the API (ms since epoch), and
add a new parameter, start_time, which is in seconds not nanoseconds.

Deprecate the old parameter, to be removed in a future release.

Also adds an complex mocked test for wait_until_done, which tests that it really is waiting for the expected time, based on the created_on field from the API, which comes back in milliseconds.


The actual src/ changes are pretty straightforward:

  • change from ns to seconds: the API returns the unix timestamp in ms and julia's time() function returns timestamp in seconds (to ~microsecond precision).

But then i made the PR more complicated by:

  • keeping the old interface to make the change backwards compatible, and
  • adding a complicated mocked unit test to make sure we're testing the behavior here and it's working as we expected

... sorry for the extra complexity.

Correctly account for the time sent from the API (ms since epoch), and
add a new parameter, start_time, which is in *seconds* not nanoseconds.

Deprecate the old parameter, to be removed in a future release.
@NHDaly NHDaly requested review from nantiamak and quinnj January 14, 2023 01:13
@NHDaly
Copy link
Member Author

NHDaly commented Jan 14, 2023

Wow, creating this unit test was a real pain, but i think it's pretty solid!

It turned out we did already have some integration tests, so i don't understand why those weren't failing before @nantiamak's PR #103... 🤔 Very strange.

But anyway, i think this should be good once and for all.

@NHDaly NHDaly requested a review from NRHelmi January 17, 2023 19:21
Copy link
Contributor

@nantiamak nantiamak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NHDaly Thanks for the PR! LGTM.

Just a comment about the description of the PR: "julia's time() function returns unix timestamp in ms." I think time() returns seconds. You are using it for seconds in the code, so I believe the code works as expected. It's just the description that needs updating, if I understand correctly.

@NHDaly
Copy link
Member Author

NHDaly commented Jan 19, 2023

Thanks you're 100% correct! I think my understanding was wrong when i started, and then i fixed it in subsequent commits. Thanks very much for the review! 😊 i'll adjust that then merge.

@NHDaly NHDaly merged commit 3958c4d into main Jan 19, 2023
@NHDaly NHDaly deleted the nhd-fix-wait_until_done-accounting branch January 19, 2023 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants