Skip to content

Fix flaky test #2835

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

Merged
merged 2 commits into from
Apr 19, 2022
Merged

Fix flaky test #2835

merged 2 commits into from
Apr 19, 2022

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Apr 18, 2022

"add missing modules (non workspace)" became flaky after the recent changes to snapshot VFS data.

The test calls createDoc to open a module B imports A where A does not exist, then waits for the missing module error and calls createDoc to open a module A. Importantly, both B and A live outside the workspace and thus are not subject to FileWatched notifications.

Unfortunately, the test wasn't testing the right thing because createDoc sends a FileWatched notification right before sending a DidOpen notification! This was causing a race condition, where the FileWatched notification triggers a build that may or may not run the FileExists ModuleA key:

  • If it does, then the key will resolve to False since ModuleA exists neither in the file system nor in VFS, and the test will fail since the key will not get rebuilt again (nothing dirties it).
  • If the build gets aborted before it does, then the key will get computed in the next build (for the DidOpen notification) and will return true because the file now exists in VFS.

I imagine that the test was passing before since the VFS state would get updated concurrently, and the race condition would not get triggered in practice.

To fix the test, we simply tell lsp-test not to send FileWatched notifications.

I also found an old comment stating that "VFS existence is not enough, a file must exist in disk". This is clearly not true anymore, so I have deleted it.

Fixes #2831

@pepeiborra pepeiborra requested a review from wz1000 April 18, 2022 09:45
Copy link
Collaborator

@wz1000 wz1000 left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this! I ran the testsuite a couple of times but could not reproduce this failure locally at all.

@michaelpj
Copy link
Collaborator

To fix the test, we simply tell lsp-test not to send FileWatched notifications.

I'm confused, that doesn't look like what the change does? And I don't see the comment you mentioned being deleted?

@pepeiborra
Copy link
Collaborator Author

I'm confused, that doesn't look like what the change does? And I don't see the comment you mentioned being deleted?

Fixed now, thanks for noticing this. I must have forgotten to git push -f

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Apr 19, 2022
@mergify mergify bot merged commit 4a5d471 into master Apr 19, 2022
sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request May 21, 2022
* Fix flaky test

* add comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test "add missing module (non workspace)" is very flaky
3 participants