Fix the flakyness of test which verifies there has to be only one npm request at a time #1026
Conversation
… request at a time
There was a problem hiding this comment.
Pull Request Overview
This PR aims to fix the flakiness of tests related to npm install requests and improve the testing utilities by adding a hook to verify npm install conditions. The key changes are:
- Refactoring internal test option fields from unexported (testOptions) to exported (TestOptions) for consistent usage.
- Introducing a CheckBeforeNpmInstall callback in the test options to synchronize and verify the state before an npm install.
- Adding a PendingRunRequestsCount method to the TypingsInstaller to assist in tracking pending npm install requests.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/testutil/projecttestutil/projecttestutil.go | Renamed testOptions to TestOptions and added CheckBeforeNpmInstall callback |
| internal/project/ata_test.go | Updated test assertions and added a hook using CheckBeforeNpmInstall to control install ordering |
| internal/project/ata.go | Added a new PendingRunRequestsCount method for testing purposes |
| }, | ||
| }, | ||
| TypingsInstallerOptions: project.TypingsInstallerOptions{ | ||
| ThrottleLimit: 1, | ||
| }, | ||
| }) | ||
|
|
||
| host.TestOptions.CheckBeforeNpmInstall = func(cwd string, npmInstallArgs []string) { |
There was a problem hiding this comment.
The loop inside the CheckBeforeNpmInstall callback may run indefinitely if the pending run requests count never becomes 1. Consider adding a timeout mechanism to avoid a potential infinite loop and to fail gracefully when the condition is not met.
| host.TestOptions.CheckBeforeNpmInstall = func(cwd string, npmInstallArgs []string) { | |
| host.TestOptions.CheckBeforeNpmInstall = func(cwd string, npmInstallArgs []string) { | |
| timeout := 5 * time.Second | |
| startTime := time.Now() |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Yeah, wat happens if the pending requests drop to 0 instead of 1?
There was a problem hiding this comment.
it wont thats what it is checking/. initially it might be 0, but we wait till we get 1 and then return from this function to continue npm install so next request cannot start till we return from here
There was a problem hiding this comment.
I guess the test infra will time out the test anyway, but it would probably be good do something like this in the loop:
assert.NilError(t, t.Context().Err()Just so we error out and bail when the test is cancelled.
There was a problem hiding this comment.
Mainly, I'm just wanting the test to not hang forever if we introduce a bug.
| host.TestOptions.CheckBeforeNpmInstall = nil // Stop checking after first run | ||
| break | ||
| } | ||
| time.Sleep(10 * time.Millisecond) |
There was a problem hiding this comment.
Can we do this without sleeps?
There was a problem hiding this comment.
Basically this thing is waiting to ensure that there is request in the queue - should i just keep looping till i see that instead of sleep ?
There was a problem hiding this comment.
Ideally we'd have a version that doesn't require sleeping, since we have nicer things available in Go for managing queues and waiting for done. Probably not possible in the current layout, at least.
No description provided.