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.

Sign up for GitHub

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

Jump to bottom

chore: Racy buffering tests correction #2106

Merged
merged 21 commits into from
Mar 23, 2020
Merged

chore: Racy buffering tests correction #2106

merged 21 commits into from
Mar 23, 2020

Conversation

MOZGIII
Copy link
Contributor

@MOZGIII MOZGIII commented Mar 20, 2020 •
edited
Loading

Closes #2102.

This solved the issue with racy buffer checking tests.
This, in turn, should fix our nightly builds! 🎉

@github-actions
Copy link

github-actions bot commented Mar 20, 2020 •
edited
Loading

Great PR! Please pay attention to the following items before merging:

Files matching src/**:

  • For each failure path, is there sufficient context logged for users to investigate the issue?
  • Do the tests ensure that behavior is sane for inputs that don't meet normal assumptions (e.g. missing field, non-string, etc)?
  • Did you add adequate documentation?

This is an automatically generated QA checklist based on modified files

@MOZGIII MOZGIII changed the title chore: Racy buffing tests correction chore: Racy buffering tests correction Mar 20, 2020
@MOZGIII MOZGIII force-pushed the tokio-compat-racy-tests-correction branch 6 times, most recently from d2e04c7 to 89486a9 Compare March 21, 2020 02:01
@MOZGIII MOZGIII mentioned this pull request Mar 21, 2020
Closed
@MOZGIII MOZGIII marked this pull request as ready for review March 21, 2020 02:07
@MOZGIII MOZGIII force-pushed the tokio-compat-racy-tests-correction branch from 89486a9 to b16d174 Compare March 21, 2020 02:21
MOZGIII added 16 commits March 21, 2020 05:22
@MOZGIII
Correct socket shutdown logic at test_util::send_lines
84a7987 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Add trace_init to buffering tests
8cfd24f 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Improve the clarity at the test_buffering test
93629eb 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Allow using tests/support/mod.rs in multiple tests
2784ba5 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Improve error event at SinkConfig::build for MockSinkConfig
45f3696 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Add support for mocking a dead sink
48b0ac2 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Require explicit sink channel size specification
1865fbb 
- remove `sink_with_buffer_size`
- add `channel_size` arg to `sink` and `sink_failing_healthcheck`

Rationale is it's otherwise unobvious how sink is implemented, and having
a hidden constant of 10 can cause confusion. It's better to force users
to put an arbitrary number there when they don't care about the channel
size - cause it's clear and explicit, compared to more concise, but
obtrusive notation. This also reduces the API surface.

Signed-off-by: MOZGIII <[email protected]>

# Conflicts:
#	tests/support/mod.rs
@MOZGIII
Convert test_buffering to using mock sources and sinks, address races
34ba76c 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Convert test_max_size to using mock sources and sinks, address races
72591c7 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Add test_util::receive_events and switched buffering tests to it
8f83809 
This is a proper way to avoid relying on the sink channel size

Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Add the ability to alter mock source data type
da971bc 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Switched sinks at test_max_size_resume to mocks, changed sleep logic
ade6821 
Before sleep was used to cope with a race condition. We don't have that
particular race anymore. We have a new one though, the same as in
`test_buffering` and `test_max_size`. However, we don't care about that one
in this test, because based on empiric checks, it doesn't seem to
contribute any meaningful difference. The cause of this race condition is
not clear so far, so it's hard to say anything certain here.

Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Unignored and updated test_reclaim_disk_space to match the test_buffe…
a5f2763 
…ring logic

Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Extract a compute_disk_size fn
c4d749c 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Clean up unused dependencies
38eb793 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII
Resolve name collision
ccd065a 
Signed-off-by: MOZGIII <[email protected]>
@MOZGIII MOZGIII force-pushed the tokio-compat-racy-tests-correction branch from b16d174 to 90bbc1e Compare March 21, 2020 02:22
Hoverbear
Hoverbear reviewed Mar 21, 2020
View reviewed changes
tests/support/mod.rs
// Using a shared mod like this is probably not the best idea, since we have to
// disable the `dead_code` lint, as we don't need all of the helpers from here
// all over the place.
#![allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused. If we have dead code why do we not just remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is this mod is used in tests, and each test is an individual compile unit. So, tests/buffering.rs complains about one set of fns as dead code, and tests/topology.rs doesn’t like the other. This is kind of a bad pattern to organize test helpers like that, but here we are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is this mod is used in tests, and each test is an individual compile unit. So, tests/buffering.rs complains about one set of fns as dead code, and tests/topology.rs doesn’t like the other. This is kind of a bad pattern to organize test helpers like that, but here we are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hoverbear we'll need to refactor it at some point. I don't want to do it as part of this PR though since it's sort of a hotfix, and we better merge is asap.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. :)

@stbrody stbrody removed their request for review March 21, 2020 21:48
@MOZGIII MOZGIII requested a review from lukesteensen March 23, 2020 14:41
@Hoverbear Hoverbear added the type: enhancement A value-adding code change that enhances its existing functionality. label Mar 23, 2020
@lukesteensen lukesteensen removed the request for review from michaelfairley March 23, 2020 16:30
LucioFranco
LucioFranco approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

much better 👍

@MOZGIII MOZGIII merged commit 2675d77 into master Mar 23, 2020
@binarylogic binarylogic deleted the tokio-compat-racy-tests-correction branch April 24, 2020 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hoverbear Hoverbear Hoverbear left review comments

@LucioFranco LucioFranco LucioFranco approved these changes

@lukesteensen lukesteensen lukesteensen approved these changes

Assignees

@Hoverbear Hoverbear

@lukesteensen lukesteensen

Labels
type: enhancement A value-adding code change that enhances its existing functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_max_size test is failing
4 participants
@MOZGIII @Hoverbear @lukesteensen @LucioFranco