From 33e9ad731d8ee749c5a4124c5355eb59afc8e767 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Tue, 3 Jun 2025 12:08:27 -0700 Subject: [PATCH 1/2] Fix the flakyness of test which verifies there has to be only one npm request at a time --- internal/project/ata.go | 6 +++ internal/project/ata_test.go | 46 +++++++++++-------- .../projecttestutil/projecttestutil.go | 25 ++++++---- 3 files changed, 47 insertions(+), 30 deletions(-) diff --git a/internal/project/ata.go b/internal/project/ata.go index 31dd9b1465..a096c5d5e3 100644 --- a/internal/project/ata.go +++ b/internal/project/ata.go @@ -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) diff --git a/internal/project/ata_test.go b/internal/project/ata_test.go index 499a7de284..0e7e75d31b 100644 --- a/internal/project/ata_test.go +++ b/internal/project/ata_test.go @@ -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" @@ -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" })) }) @@ -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{ @@ -250,7 +248,7 @@ 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{ @@ -258,23 +256,31 @@ func TestAta(t *testing.T) { }, }) + host.TestOptions.CheckBeforeNpmInstall = func(cwd string, npmInstallArgs []string) { + 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 + } + time.Sleep(10 * time.Millisecond) + } + } + 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) { diff --git a/internal/testutil/projecttestutil/projecttestutil.go b/internal/testutil/projecttestutil/projecttestutil.go index bf6edf7072..6c5f8093ca 100644 --- a/internal/testutil/projecttestutil/projecttestutil.go +++ b/internal/testutil/projecttestutil/projecttestutil.go @@ -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 { @@ -35,7 +36,7 @@ type ProjectServiceHost struct { output strings.Builder logger *project.Logger ClientMock *ClientMock - testOptions *TestTypingsInstallerOptions + TestOptions *TestTypingsInstallerOptions ServiceOptions *project.ServiceOptions } @@ -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 { @@ -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 } @@ -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) } @@ -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++ } From c97951d647800d115d183493b0bca620b22ebf77 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Tue, 3 Jun 2025 14:18:12 -0700 Subject: [PATCH 2/2] Feedback --- internal/project/ata_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/project/ata_test.go b/internal/project/ata_test.go index 0e7e75d31b..b0ce457c55 100644 --- a/internal/project/ata_test.go +++ b/internal/project/ata_test.go @@ -266,6 +266,7 @@ func TestAta(t *testing.T) { host.TestOptions.CheckBeforeNpmInstall = nil // Stop checking after first run break } + assert.NilError(t, t.Context().Err()) time.Sleep(10 * time.Millisecond) } }