Skip to content

Fix the flakyness of test which verifies there has to be only one npm request at a time #1026

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

Merged
merged 2 commits into from
Jun 3, 2025
Merged

Fix the flakyness of test which verifies there has to be only one npm request at a time #1026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33e9ad7
Fix the flakyness of test which verifies there has to be only one npm…
sheetalkamat Jun 3, 2025
c97951d
Feedback
sheetalkamat Jun 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions internal/project/ata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ type TypingsInstaller struct {
pendingRunRequestsMu sync.Mutex
}

func (ti *TypingsInstaller) PendingRunRequestsCount() int {
ti.pendingRunRequestsMu.Lock()
defer ti.pendingRunRequestsMu.Unlock()
return len(ti.pendingRunRequests)
}

func (ti *TypingsInstaller) IsKnownTypesPackageName(p *Project, name string) bool {
// We want to avoid looking this up in the registry as that is expensive. So first check that it's actually an NPM package.
validationResult, _, _ := ValidatePackageName(name)
Expand Down
47 changes: 27 additions & 20 deletions internal/project/ata_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package project_test
import (
"slices"
"testing"
"time"

"github.com/microsoft/typescript-go/internal/bundled"
"github.com/microsoft/typescript-go/internal/core"
Expand Down Expand Up @@ -215,16 +216,12 @@ func TestAta(t *testing.T) {
_, p2 := service.EnsureDefaultProjectForFile("/user/username/projects/project2/app.js")
var installStatuses []project.TypingsInstallerStatus
installStatuses = append(installStatuses, <-host.ServiceOptions.InstallStatus, <-host.ServiceOptions.InstallStatus)
// Order can be non deterministic since they both will run in parallel
assert.Assert(t, slices.Contains(installStatuses, project.TypingsInstallerStatus{
RequestId: 1,
Project: p1,
Status: "Success",
// Order can be non deterministic since they both will run in parallel - not looking into request ID
assert.Assert(t, slices.ContainsFunc(installStatuses, func(s project.TypingsInstallerStatus) bool {
return s.Project == p1 && s.Status == "Success"
}))
assert.Assert(t, slices.Contains(installStatuses, project.TypingsInstallerStatus{
RequestId: 2,
Project: p2,
Status: "Success",
assert.Assert(t, slices.ContainsFunc(installStatuses, func(s project.TypingsInstallerStatus) bool {
return s.Project == p2 && s.Status == "Success"
}))
})

Expand All @@ -242,6 +239,7 @@ func TestAta(t *testing.T) {
"typeAcquisition": { "include": ["grunt", "gulp", "commander"] },
}`,
}
expectedP1First := true
service, host := projecttestutil.Setup(files, &projecttestutil.TestTypingsInstaller{
TestTypingsInstallerOptions: projecttestutil.TestTypingsInstallerOptions{
PackageToFile: map[string]string{
Expand All @@ -250,31 +248,40 @@ func TestAta(t *testing.T) {
"lodash": "declare const lodash: { x: number }",
"cordova": "declare const cordova: { x: number }",
"grunt": "declare const grunt: { x: number }",
"gulp": "declare const grunt: { x: number }",
"gulp": "declare const gulp: { x: number }",
},
},
TypingsInstallerOptions: project.TypingsInstallerOptions{
ThrottleLimit: 1,
},
})

host.TestOptions.CheckBeforeNpmInstall = func(cwd string, npmInstallArgs []string) {
Copy link
Preview

Copilot AI Jun 3, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
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.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, wat happens if the pending requests drop to 0 instead of 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Mainly, I'm just wanting the test to not hang forever if we introduce a bug.

for {
pendingCount := service.TypingsInstaller().PendingRunRequestsCount()
if pendingCount == 1 {
if slices.Contains(npmInstallArgs, "@types/gulp@latest") {
expectedP1First = false
}
host.TestOptions.CheckBeforeNpmInstall = nil // Stop checking after first run
break
}
assert.NilError(t, t.Context().Err())
time.Sleep(10 * time.Millisecond)
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this without sleeps?

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
}

service.OpenFile("/user/username/projects/project1/app.js", files["/user/username/projects/project1/app.js"].(string), core.ScriptKindJS, "")
service.OpenFile("/user/username/projects/project2/app.js", files["/user/username/projects/project2/app.js"].(string), core.ScriptKindJS, "")
_, p1 := service.EnsureDefaultProjectForFile("/user/username/projects/project1/app.js")
_, p2 := service.EnsureDefaultProjectForFile("/user/username/projects/project2/app.js")
// Order is determinate since second install will run only after completing first one
status := <-host.ServiceOptions.InstallStatus
assert.Equal(t, status, project.TypingsInstallerStatus{
RequestId: 1,
Project: p1,
Status: "Success",
})
assert.Equal(t, status.Project, core.IfElse(expectedP1First, p1, p2))
assert.Equal(t, status.Status, "Success")
status = <-host.ServiceOptions.InstallStatus
assert.Equal(t, status, project.TypingsInstallerStatus{
RequestId: 2,
Project: p2,
Status: "Success",
})
assert.Equal(t, status.Project, core.IfElse(expectedP1First, p2, p1))
assert.Equal(t, status.Status, "Success")
})

t.Run("discover from node_modules", func(t *testing.T) {
Expand Down
25 changes: 15 additions & 10 deletions internal/testutil/projecttestutil/projecttestutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,9 @@ import (
//go:generate go tool github.com/matryer/moq -stub -fmt goimports -pkg projecttestutil -out clientmock_generated.go ../../project Client

type TestTypingsInstallerOptions struct {
TypesRegistry []string
PackageToFile map[string]string
TypesRegistry []string
PackageToFile map[string]string
CheckBeforeNpmInstall func(cwd string, npmInstallArgs []string)
}

type TestTypingsInstaller struct {
Expand All @@ -35,7 +36,7 @@ type ProjectServiceHost struct {
output strings.Builder
logger *project.Logger
ClientMock *ClientMock
testOptions *TestTypingsInstallerOptions
TestOptions *TestTypingsInstallerOptions
ServiceOptions *project.ServiceOptions
}

Expand Down Expand Up @@ -89,7 +90,7 @@ var _ project.ServiceHost = (*ProjectServiceHost)(nil)
func Setup(files map[string]any, testOptions *TestTypingsInstaller) (*project.Service, *ProjectServiceHost) {
host := newProjectServiceHost(files)
if testOptions != nil {
host.testOptions = &testOptions.TestTypingsInstallerOptions
host.TestOptions = &testOptions.TestTypingsInstallerOptions
}
var throttleLimit int
if testOptions != nil && testOptions.ThrottleLimit != 0 {
Expand All @@ -112,7 +113,7 @@ func Setup(files map[string]any, testOptions *TestTypingsInstaller) (*project.Se
}

func (p *ProjectServiceHost) NpmInstall(cwd string, npmInstallArgs []string) ([]byte, error) {
if p.testOptions == nil {
if p.TestOptions == nil {
return nil, nil
}

Expand All @@ -127,10 +128,14 @@ func (p *ProjectServiceHost) NpmInstall(cwd string, npmInstallArgs []string) ([]
return nil, err
}

if p.TestOptions.CheckBeforeNpmInstall != nil {
p.TestOptions.CheckBeforeNpmInstall(cwd, npmInstallArgs)
}

for _, atTypesPackageTs := range npmInstallArgs[2 : lenNpmInstallArgs-2] {
// @types/packageName@TsVersionToUse
packageName := atTypesPackageTs[7 : len(atTypesPackageTs)-len(project.TsVersionToUse)-1]
content, ok := p.testOptions.PackageToFile[packageName]
content, ok := p.TestOptions.PackageToFile[packageName]
if !ok {
return nil, fmt.Errorf("content not provided for %s", packageName)
}
Expand Down Expand Up @@ -187,12 +192,12 @@ func TypesRegistryConfig() map[string]string {
func (p *ProjectServiceHost) createTypesRegistryFileContent() string {
var builder strings.Builder
builder.WriteString("{\n \"entries\": {")
for index, entry := range p.testOptions.TypesRegistry {
for index, entry := range p.TestOptions.TypesRegistry {
appendTypesRegistryConfig(&builder, index, entry)
}
index := len(p.testOptions.TypesRegistry)
for key := range p.testOptions.PackageToFile {
if !slices.Contains(p.testOptions.TypesRegistry, key) {
index := len(p.TestOptions.TypesRegistry)
for key := range p.TestOptions.PackageToFile {
if !slices.Contains(p.TestOptions.TypesRegistry, key) {
appendTypesRegistryConfig(&builder, index, key)
index++
}
Expand Down