Fix flaky unit test: linked_module_resolver_test

[c0d1ngm0nk3y]

Summary

The unit test linked_module_resolver_test was flaky.

Use Cases

Checklist

• I have viewed, signed, and submitted the Contributor License Agreement.
• I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
• I have added an integration test, if necessary.
• I have reviewed the styleguide for guidance on my code quality.
• I'm happy with the commit history on this PR (I have rebased/squashed as needed).

[c0d1ngm0nk3y]

@ryanmoran Could you have a look?

[c0d1ngm0nk3y]

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

[c0d1ngm0nk3y]

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.

[mhdawson]

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.

[c0d1ngm0nk3y]

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

[pacostas]

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.

[pacostas]

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.

[c0d1ngm0nk3y]

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.

[c0d1ngm0nk3y]

Is there anything wrong/missing with this pr? It looks like the flake is already in main for month.

[pacostas]

Is there anything wrong/missing with this pr? It looks like the flake is already in main for month.

@c0d1ngm0nk3y We were looking at the issue with @mhdawson. We concluded that it is preferred to keep the structure of the test data as the rationale of the author who wrote the test, seems to be on erroring over wrong permissions, so we think its good to follow the same pattern instead of testing it over something new.
So instead of creating a file and erroring over that, you can change the permissions of the directory where the module is trying to create the subsequent directories.

This would mean, that for the first flaky test on line 132 you can do

			context("when the destination cannot be scaffolded", func() {
				it.Before(func() {
					// Module 1 to 4 are written under sub-dir
					Expect(os.Mkdir(filepath.Join(layerPath, "sub-dir"), 0400)).To(Succeed())
					// Module 5 is written under layerPath
					Expect(os.Chmod(layerPath, 0400)).To(Succeed())
				})

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

and for the second flaky test on line 216 you can do

			context("when the destination cannot be scaffolded", func() {
				it.Before(func() {
					// Module 1 to 4 are written under sub-dir
					Expect(os.Mkdir(filepath.Join(otherLayerPath, "sub-dir"), 0400)).To(Succeed())
					// Module 5 is written under otherLayerPath
					Expect(os.Chmod(otherLayerPath, 0400)).To(Succeed())
				})

				it("returns an error", func() {
					err := resolver.Resolve(filepath.Join(workspace, "package-lock.json"), filepath.Join(otherLayerPath, "sub-dir"))
					Expect(err).To(MatchError(ContainSubstring("failed to setup linked module directory scaffolding")))
				})

				it.After(func() {
					Expect(os.Chmod(filepath.Join(otherLayerPath), 0700)).To(Succeed())
				})
			})

[c0d1ngm0nk3y]

We concluded that it is preferred to keep the structure of the test data as the rationale of the author who wrote the test, seems to be on erroring over wrong permissions, so we think its good to follow the same pattern instead of testing it over something new.

But the test is indeterministic because of the way golang loops over dicts. The author of this test clearly wanted to target a very specific error and the very same error will be raised regardless on how the error is provoked.

Any additional information should be reflected in some form in the test imo.

[c0d1ngm0nk3y]

I changed the fix to address the underlying problem. The testdata containes several modules which are looped over in a non-deterministic order. Not ideal for an error test. I reduced the testdata for those cases, making tests more readable imho.

@paketo-buildpacks/nodejs-maintainers Could you have another look?

[pacostas]

LGTM

[mhdawson]

LGTM