Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix flaky test #4547

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
andyleejordan merged 2 commits into main from andschwa/flaky-test
Apr 26, 2023
Merged

Fix flaky test #4547

andyleejordan merged 2 commits into main from andschwa/flaky-test
Apr 26, 2023

Conversation

Copy link
Member

@andyleejordan andyleejordan commented Apr 26, 2023

I believe this test was flaky due to race conditions caused by the events. Awaiting promises instead should eliminate those races.

I believe this test was flaky due to race conditions caused by the events.
Awaiting promises instead should eliminate those races.
Copy link
Member Author

Let's see if they all pass.

Copy link
Collaborator

I believe this test was flaky due to race conditions caused by the events. Awaiting promises instead should eliminate those races.

Events are so hard to test. I think it might still race flake so we can try using the done() callback in the event to make sure it doesn't end until all the assertions are complete. I'll take a look

Copy link
Member Author

I did also increase the timeout in the release PR while I was adding a skip:

Binary Modules
 ✔ Debugs a binary module script (11974ms)
 ✔ Stops at a binary module breakpoint (10853ms)

But IIRC it was set to 20m000ms and they're coming in around 10,000ms.

Anyway, I'm down to merge this and let upcoming work test the flakiness. They passed on the first try this time!

@andyleejordan andyleejordan enabled auto-merge (squash) April 26, 2023 18:44
Copy link
Member Author

Re-running checks to see if they pass again.

Copy link
Member Author

Events are so hard to test. I think it might still race flake so we can try using the done() callback in the event to make sure it doesn't end until all the assertions are complete. I'll take a look

That's essentially what the Promise does. By doing the assignment within the event as a resolution to the promise, the latter test code which needs the value waits for that resolved promise (and thus assignment) to actually happen (which is in the background asynchronously via the event). So instead of racing the event to its completion, the test code awaits for that logic to happen.

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member Author

2 failing

  1. RunCode feature
    "before all" hook: ensureEditorServicesIsConnected for "Creates the launch config":
    Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/runner/work/1/s/vscode-powershell/out/test/features/RunCode.test.js)
    at listOnTimeout (node:internal/timers:559:17)
    at process.processTimers (node:internal/timers:502:7)

  2. ISE compatibility feature
    "before all" hook in "ISE compatibility feature":
    Error: The extension 'ms-vscode.powershell' is already registered.
    at xs.registerExternalExtension (out/main.js:87:30059)
    at Object.registerExternalExtension (out/main.js:91:1370)
    at Object.ensureEditorServicesIsConnected (out/test/utils.js:85:33)
    at async Context. (out/test/features/ISECompatibility.test.js:40:9)

Grr, we need more timeout for the before.

Copy link
Member Author

All right, I think we hit bingo!

@andyleejordan andyleejordan deleted the andschwa/flaky-test branch April 26, 2023 19:40
Copy link
Collaborator

JustinGrote commented Apr 26, 2023
edited
Loading

Events are so hard to test. I think it might still race flake so we can try using the done() callback in the event to make sure it doesn't end until all the assertions are complete. I'll take a look

That's essentially what the Promise does. By doing the assignment within the event as a resolution to the promise, the latter test code which needs the value waits for that resolved promise (and thus assignment) to actually happen (which is in the background asynchronously via the event). So instead of racing the event to its completion, the test code awaits for that logic to happen.

Makes sense, I have some other ideas but LGTM! Thanks.

andyleejordan reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SeeminglyScience SeeminglyScience SeeminglyScience approved these changes

@JustinGrote JustinGrote Awaiting requested review from JustinGrote

Assignees
No one assigned
Labels
Area-Test Issue-Bug A bug to squash.
Projects
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /