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

Fix test_vfs_works failing on Windows due to extra Write events #830

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

vipentti
Copy link
Contributor

On Windows notify generates extra Write events for folders, which caused
process_tasks to not handle all tasks generated on Windows.

This fixes #827

On Windows `notify` generates extra `Write` events for folders, which caused
`process_tasks` to not handle all tasks generated on Windows.

This fixes rust-lang#827
@matklad
Copy link
Member

matklad commented Feb 14, 2019

r? @pnkfelix

What do you think about such fix? I think it's ok to be OS-specific here, to guarantee that we process all events.

@pnkfelix
Copy link
Member

Hmm. If your goal is to process "all" pending events, why not loop without any bound on task count until you hit the first timeout, and then return in response to the timeout?

The calling code will then be responsible for checking that the events that were processed actually cover the behavior of interest, right?

@pnkfelix
Copy link
Member

pnkfelix commented Feb 14, 2019

Having said that, its looking like these tests are going to end up piling up hack-after-os-specific-hack anyway, so I'm not saying the approach of passing 4-vs-2 is unusable. I'm just want to understand what's wrong with the approach I outlined, which seems slightly more general.

@pnkfelix
Copy link
Member

(I guess awaiting a timeout on every call to process_events could increase timing time by an unacceptable amount...)

@matklad
Copy link
Member

matklad commented Feb 14, 2019

I like 2 vs 4 for two reasons:

  • it is faster, especially given that the same timeout will be used on fast local machine and on the potentially slow virtualized CI

  • it is less flexible in a sense that we better constrain the set of possible outcomes.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 14, 2019

My only other misgiving is that I don't understand the claim that this guarantees that we are "processing all events". By definition, the given approach just covers a fixed number (where that fixed number probably matches the total number of events on the platforms that people have bothered to check it).

  • But that's fine, in terms of practicality. it reminds me of the tricks people use to turn a given Liveness property into a Safety property in distributed computing.

In other words, its just a best-effort test. Its hard to make a sensible cross-platform test here.

So I'll accept the hack, and maybe add further ones along these lines later once I finish figuring out the Mac OS X issues.

@pnkfelix
Copy link
Member

bors r+

bors bot added a commit that referenced this pull request Feb 14, 2019
830: Fix test_vfs_works failing on Windows due to extra Write events r=pnkfelix a=vipentti

On Windows `notify` generates extra `Write` events for folders, which caused
`process_tasks` to not handle all tasks generated on Windows.

This fixes #827

Co-authored-by: Ville Penttinen <[email protected]>
@bors
Copy link
Contributor

bors bot commented Feb 14, 2019

Build succeeded

@bors bors bot merged commit 50e49e0 into rust-lang:master Feb 14, 2019
@pnkfelix
Copy link
Member

My only other misgiving is that I don't understand the claim that this guarantees that we are "processing all events". By definition, the given approach just covers a fixed number (where that fixed number probably matches the total number of events on the platforms that people have bothered to check it).

Hmm, I just thought of one way that might be able to check for this ... though it may depend on details of how the vfs task_receiver result actually works: After finishing the loop in fn process_tasks over the given number of tasks (i.e. num_tasks), we could then add an extra call to recv_timeout with a short duration, and assert that it always times out.

This would act as a guard against the scenario where there was actually an unexpected event pending.

@matklad
Copy link
Member

matklad commented Feb 14, 2019

Another options is to check that the event queue is empty at the very end of the test,once

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.

test_vfs_works fails on Windows
3 participants