Skip to content
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

Fix flaky unit test: linked_module_resolver_test #689

Merged
merged 2 commits into from
Jun 4, 2024
Merged

Fix flaky unit test: linked_module_resolver_test #689

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
310195e
Fix flaky unit test: linked_module_resolver_test
nicolasbender May 17, 2024
cef9d76
Make unit test deterministic
c0d1ngm0nk3y Jun 3, 2024
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
20 changes: 20 additions & 0 deletions linked_module_resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,16 @@ func testLinkedModuleResolver(t *testing.T, context spec.G, it spec.S) {

context("when the destination cannot be scaffolded", func() {
it.Before(func() {
err := os.WriteFile(filepath.Join(workspace, "package-lock.json"), []byte(`{
"packages": {
"module": {
"resolved": "src/packages/module",
"link": true
}
}
}`), 0600)
Expect(err).NotTo(HaveOccurred())

Expect(os.Mkdir(filepath.Join(layerPath, "sub-dir"), 0400)).To(Succeed())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh: no idea why the test succeeded "most of the time"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After "some" debugging: It worked in 2/3 of the cases because the resolved path in the test data has some subfolder.

1 of the 3 links is without subfolders and iterating over a dict in go doesn't give a guaranteed order.

MkDirAll will not fail for a r/o folder if the folder already exist and no subfolder is needed.

Copy link
Member

Choose a reason for hiding this comment

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

Using WriteFile seems different than creating a sub-directory. Do you have the output from when it fails? That would be good to understand why it was flaky and what the right fix might be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using WriteFile seems different than creating a sub-directory.

It is. That is the reason why it works now :) Imho, WriteFile is the correct tool for that anyway. Unfortunately, the tests are not that self explanatory and we had to make some assumptions. But in the end all made sense.

Do you have the output from when it fails?

You can just look in the pr validation of some other prs, e.g. #688 (it roughly happes 1/3 of the time)

install/LinkedModuleResolver/Copy/failure_cases/when_the_destination_cannot_be_scaffolded/returns_an_error
    linked_module_resolver_test.go:217: 
        Expected
            <*fmt.wrapError | 0xc000312640>: 
            failed to copy linked module directory to layer path: failed to copy: destination exists: remove /tmp/another-layer1310829255/sub-dir/module-5: permission denied
            {
                msg: "failed to copy linked module directory to layer path: failed to copy: destination exists: remove /tmp/another-layer1310829255/sub-dir/module-5: permission denied",
                err: <*fmt.wrapError | 0xc000312620>{
                    msg: "failed to copy: destination exists: remove /tmp/another-layer1310829255/sub-dir/module-5: permission denied",
                    err: <*fs.PathError | 0xc0004f50b0>{
                        Op: "remove",
                        Path: "/tmp/another-layer1310829255/sub-dir/module-5",
                        Err: <syscall.Errno>0xd,
                    },
                },
            }
        to match error
            <*matchers.ContainSubstringMatcher | 0xc0004f50e0>: {
                Substr: "failed to setup linked module directory scaffolding",
                Args: nil,
            }

That would be good to understand why it was flaky and what the right fix might be.

We thought so as well, so after fixing it, we had a "little" debugging session to understand why the test is flaky. In a nutshell:

  • The test creates a r/o directory to provoke a very specific error when a subdirectory is created via MkDirAll
  • In go, iterating over a dict is not ordered, but the order is random
  • The test data needs to create links in a subdirectory that cannot be created
  • EXCEPT: One module (module-5) that doesn't need a subdirectory
  • The call will still fail, but with a different error because the r/o directory should be removed

When creating a file instead of directory, it works

  • You cannot create a subdirectory to a file
  • MkDirAll fails if you want to create a directory with the same name as a file

Copy link
Contributor

@pacostas pacostas May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the test is flaky.
On this part here

npm-install/linked_module_resolver_test.go

Line 134 in 7b388f0

Expect(os.Mkdir(filepath.Join(layerPath, "sub-dir"), 0400)).To(Succeed())
it sets the permissions.
If the first iteration that happens here

npm-install/linked_module_resolver.go

Line 58 in 7b388f0

err = os.MkdirAll(filepath.Dir(destination), os.ModePerm)
the module is anything else except the module-5

npm-install/linked_module_resolver_test.go

Lines 45 to 67 in 7b388f0

err = os.WriteFile(filepath.Join(workspace, "package-lock.json"), []byte(`{
"packages": {
"module-1": {
"resolved": "src/packages/module-1",
"link": true
},
"module-2": {
"resolved": "http://example.com/module-2.tgz"
},
"module-3": {
"resolved": "workspaces/example/module-3",
"link": true
},
"module-4": {
"resolved": "http://example.com/module-4.tgz"
},
"module-5": {
"resolved": "module-5",
"link": true
}
}
}`), 0600)
Expect(err).NotTo(HaveOccurred())
it manages to error on the first loop as it is not able to create the directory /tmp/layer2780743255/sub-dir/src/packages as the sub-dir has permission 0400 , this is what we expect and as a result the test passes. But in case of the module-5 it will try to create the directory /tmp/layer2780743255/sub-dir but because of the fact that it has sufficient permissions for the /tmp/layer2780743255 directory

npm-install/linked_module_resolver_test.go

Line 27 in 7b388f0

layerPath, err = os.MkdirTemp("", "layer")
the mkdirAll function will not fail and will fail on the next if statement

npm-install/linked_module_resolver.go

Line 63 in 7b388f0

err = fs.Copy(source, destination)
as a result throwing a different error than the expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

So another solution in the current context could be

			context("when the destination cannot be scaffolded", func() {
				it.Before(func() {
					Expect(os.MkdirAll(filepath.Join(layerPath, "sub-dir", "another-sub-dir"), os.ModePerm)).To(Succeed())
					Expect(os.Chmod(filepath.Join(layerPath, "sub-dir"), 0400)).To(Succeed())
				})

				it("returns an error", func() {
					err := resolver.Resolve(filepath.Join(workspace, "package-lock.json"), filepath.Join(layerPath, "sub-dir", "another-sub-dir"))
					Expect(err).To(MatchError(ContainSubstring("failed to setup linked module directory scaffolding")))
				})
                                // Revert the permissions to allow proper clean up
				it.After(func() {
					Expect(os.Chmod(filepath.Join(layerPath, "sub-dir"), 0700)).To(Succeed())
				})
			})

Which practically creates a sub-sub-directory with the proper permissions in order for the module-5 to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So another solution in the current context could be

Are you sure that would fix the problem?

npm-install/linked_module_resolver.go

Line 63 in 7b388f0

err = fs.Copy(source, destination)

Would still work because the directory already exists. I haven't checked it yet, though.

})

Expand Down Expand Up @@ -209,6 +219,16 @@ func testLinkedModuleResolver(t *testing.T, context spec.G, it spec.S) {

context("when the destination cannot be scaffolded", func() {
it.Before(func() {
err := os.WriteFile(filepath.Join(workspace, "package-lock.json"), []byte(`{
"packages": {
"module": {
"resolved": "src/packages/module",
"link": true
}
}
}`), 0600)
Expect(err).NotTo(HaveOccurred())

Expect(os.Mkdir(filepath.Join(otherLayerPath, "sub-dir"), 0400)).To(Succeed())
})

Expand Down
Loading