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

Flaky test fix #71

Merged
merged 1 commit into from
Jul 14, 2024
Merged

Flaky test fix #71

merged 1 commit into from
Jul 14, 2024

Conversation

rustworthy
Copy link
Collaborator

@rustworthy rustworthy commented Jun 22, 2024

This change is Reviewable

@rustworthy rustworthy marked this pull request as draft June 22, 2024 09:11
@rustworthy rustworthy marked this pull request as ready for review June 22, 2024 09:15
@rustworthy rustworthy requested a review from jonhoo June 22, 2024 09:15
@rustworthy rustworthy mentioned this pull request Jul 6, 2024
4 tasks
Comment on lines +131 to +133
// more or less guaranteed thing is that there is one pending job in our _local_ queue. It is "more or less"
// because another test _may_ still push onto and consume from this queue if we copypasta this test's
// contents and forget to update the local queue name, i.e. do not guarantee isolation at the queues level.
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should instead work pretty hard to ensure that tests indeed have different queue names, otherwise we're likely to run into much larger and hard-to-debug issues down the line. Happy to merge this as-is to resolve the flakiness, but if we can make the assertions stronger by upholding the invariant that every test has its own unique queue name, that's something I'd like us to do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's true and I guess we are already doing so using local queue names. In this case though the server_state.data.total_enqueued is the total number of currently enqueued jobs in the system rather than in a specific queue.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, true — may be worth calling that out explicitly here

@jonhoo jonhoo merged commit b3bf1ac into main Jul 14, 2024
19 checks passed
@rustworthy rustworthy deleted the chore/cleanup branch August 9, 2024 11:27
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