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

Update async processor tests #2577

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

rafal-ch
Copy link
Contributor

Closes #2449
Or at least, tries to :)

Description

Summary of changes, each in separate commit for clarity. The change that attempts to fix the actual flakiness reported in the issue in pt. 2 below:

  1. Modify one_spawn_single_tasks_works() to not use sleep
  2. Updates assertion in one_spawn_single_tasks_works__thread_id_is_different_than_main()
    • The original test expected the exact number of worker threads to be spawned (assert_eq!(unique_thread_ids.len(), number_of_threads);), however, the AsyncProcessor can potentially reuse threads if the tasks finish quick enough.
    • One way to reduce the flakiness would be to bump the sleep, but I think the more clear way is to ensure that some (but not too many) worker threads have been spawned
      • Especially because the aim of the test is not to ensure the number of worker threads, but to check that main thread id was not used
  3. Reduces the arbitrary number of tasks in executes_10_tasks_for_10_seconds_with_one_thread, just to make the test execute faster
  4. Add some clarifying comments
  5. Slightly update and rename the executes_10_tasks_for_2_seconds_with_10_thread() and executes_10_tasks_for_2_seconds_with_1_thread() - to use proper values and better reflect the idea behind the test

Before requesting review

  • I have reviewed the code myself

@rafal-ch rafal-ch changed the title Rafal/2449 update async processor tests Update async processor tests Jan 15, 2025
@rafal-ch rafal-ch added the no changelog Skip the CI check of the changelog modification label Jan 15, 2025
@rafal-ch rafal-ch marked this pull request as ready for review January 16, 2025 11:14
@rafal-ch rafal-ch requested a review from AurelienFT January 16, 2025 11:14
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking the time to (try to) stabilize this :)

// should complete in approximately 5 seconds overall.
// Allowing some LEEWAY to account for runtime overhead.
const LEEWAY: Duration = Duration::from_millis(300);
assert!(instant.elapsed() < Duration::from_secs(5) + LEEWAY);
Copy link
Contributor

Choose a reason for hiding this comment

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

If the test complete them all in 1 seconds because it spawned too many thread it's an issue I think an other assert could be added to ensure it :

Suggested change
assert!(instant.elapsed() < Duration::from_secs(5) + LEEWAY);
assert!(instant.elapsed() < Duration::from_secs(5) + LEEWAY);
assert!(instant.elapsed() > Duration::from_secs(5));

I don't think thr LEEWAY is necessary here because it should never take less than 5 seconds in my mind but if you think there is an expected case then i'm ok to add the - LEEWAY in second assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the additional assert, added in this commit: 8443c00
However, I used >= to cover the super unlikely case that the execution took exactly 5 secs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think thr LEEWAY is necessary here because it should never take less than 5 seconds in my mind but if you think there is an expected case then i'm ok to add the - LEEWAY in second assert.

Yes, it will never take less that 5 secs. on traditional computers :-)
So the elapsed could be, let's say 5.11 sec, and I wanted to check if it's not bigger, let's say, 7 sec, because ultimately the correct behavior of this test is to execute for just slightly more than 5 secs.

I could do

assert!(instant.elapsed() >= Duration::from_secs(5));
assert!(instant.elapsed() < Duration::from_secs(6));

but I don't like the additional magic number (6) and I thought that the concept of leeway would be more clear.

Let me know if this makes any sense in the context of async processor :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah yeah i like the leeway in the first assert :)

@rafal-ch rafal-ch requested a review from AurelienFT January 16, 2025 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Skip the CI check of the changelog modification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix flaky one_spawn_single_tasks_works__thread_id_is_different_than_main test
2 participants