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

Author: sheetalkamat

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)

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,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) {
+			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)
+			}
+		}
+
 		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) {

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++
 		}