Skip to content
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

relax measure macros test check #3517

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

bw-solana
Copy link

@bw-solana bw-solana commented Nov 7, 2024

Problem

These tests rely on threads sleeping for a specific amount of time. This is dubious in general given how these sleeps work:

Puts the current thread to sleep for at least the specified amount of time.

The thread may sleep longer than the duration specified due to scheduling specifics or platform-dependent functionality. It will never sleep less.

There is a 1% margin that is added for waking up late. This seems to be sufficient in most cases, but I've seen failures: https://buildkite.com/anza/agave/builds/13456#019303ec-ef7e-444c-bb01-3060b863270b

I noticed these tests are not marked to run in serial, so I'm assuming there is the possibility of other tests running and increasing the likelihood of threads sleeping beyond the specified limit.

Also, the assert message does not indicate what the actual time was, which is less than ideal for debugging:

--- STDERR:              solana-measure macros::tests::test_measure_us_macro ---
--
  | thread 'macros::tests::test_measure_us_macro' panicked at measure/src/macros.rs:188:13:
  | assertion failed: (999_000..=1_010_000).contains(&measure)

Summary of Changes

  1. Make these tests run in serial
  2. Add assert messages to indicate what the measured time is

@bw-solana bw-solana marked this pull request as ready for review November 7, 2024 16:24
@apfitzge
Copy link

apfitzge commented Nov 7, 2024

ngl I hate time/sleep in tests in general. do these actually add value? why not just make sure the time is non-zero i.e. there is some time passing?

@bw-solana
Copy link
Author

ngl I hate time/sleep in tests in general. do these actually add value? why not just make sure the time is non-zero i.e. there is some time passing?

I'm not opposed to that. I guess the only thing we'd be missing is sanity checking that measure's time keeping somewhat matches reality.

Looks like we are just using std::time::Instant under the hood, so not much value in validating that. I was then going to make a comment indicating we probably never change these, but then I noticed #3043

@apfitzge
Copy link

apfitzge commented Nov 7, 2024

I think it's unlikely we move away from Instant though. and so long as we make sure time is non-zero in these tests it still should give some test reliability

@bw-solana
Copy link
Author

@apfitzge it is done

@apfitzge
Copy link

apfitzge commented Nov 7, 2024

🪦 sorry. Can we not wait for so long now? Just sleep for a few mics and then assert it's not 0. No need to make the test a whole second now

@bw-solana
Copy link
Author

🪦 sorry. Can we not wait for so long now? Just sleep for a few mics and then assert it's not 0. No need to make the test a whole second now

You're the worst. Shortened to 1ms

Copy link

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Change the PR title and we're good to go

@bw-solana bw-solana changed the title make measurement tests serial relax measure macros test check Nov 7, 2024
@bw-solana bw-solana merged commit a0b2c73 into anza-xyz:master Nov 7, 2024
40 checks passed
@bw-solana bw-solana deleted the measure_macro_tests branch November 7, 2024 20:35
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