Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Datetime test has a race condition? #439

Open
penberg opened this issue Dec 11, 2024 · 3 comments
Open

Datetime test has a race condition? #439

penberg opened this issue Dec 11, 2024 · 3 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested

Comments

@penberg
Copy link
Collaborator

penberg commented Dec 11, 2024

The CI caught a failure like this, which looks like a race condition in the test itself:

failures:

---- vdbe::datetime::tests::test_valid_get_time_from_datetime_value stdout ----
thread 'vdbe::datetime::tests::test_valid_get_time_from_datetime_value' panicked at core\vdbe\datetime.rs:833:17:
assertion `left == right` failed
  left: "08:47:42"
 right: "08:47:41"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
@penberg penberg added bug Something isn't working help wanted Extra attention is needed good first issue Good for newcomers question Further information is requested labels Dec 11, 2024
@ktfth
Copy link

ktfth commented Dec 11, 2024

I have worked on a PR to a possible fix: #443

@ktfth
Copy link

ktfth commented Dec 12, 2024

@penberg I'm working on another PR, made a reset and runned the test again, and the case not failed. Exists some method to reproduce the race condition?

@jabley
Copy link

jabley commented Dec 14, 2024

This is non-determinism in the test. It calls the system clock to get a time value which is then later compared with a subsequent call to the system clock to get a time value. It looks like the first call happened during one second and the subsequent call happened in a later second, hence the test failure.

I've not looked yet at how best to make the test deterministic. Trying to make the 2 calls closer together might reduce the likelihood of this failure happening, but it could still happen. A better approach would probably be to expose the system clock to stubbing out so that you can precisely define the values it will have at both times-of-use, but I've not thought through what that API might look like. I have done that in personal projects, passing a fn() -> DateTime<Utc> or similar around, but it can get a bit messy.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants