canonical / chisel

GNU Affero General Public License v3.0
270 stars 42 forks source link

Set umask in tests to 0002 #32

Closed woky closed 2 years ago

woky commented 2 years ago

deb/extract and fsutil/create tests expect umask 0002. This depends on environment so set it explicitly in tests.

cjdcordeiro commented 2 years ago

how did you encounter the issue? (I'm trying to reproduce the problem without umask)

woky commented 2 years ago

how did you encounter the issue? (I'm trying to reproduce the problem without umask)

One day I tried to run chisel tests on my workstation at home which runs ArchLinux, which has default umask of 022. Ubuntu has 002.

woky commented 2 years ago

Again, per review in the other PR, it should not depend on umask. The permissions that come in the package should be respected as-is.

The problem is with the test. It creates files with wrong permissions when umask from the environment is not set to 022, and then, the comparison fails. I think it's in archive_test.go but I can't test it now, because it fails on

$ go test ./internal/archive/
# github.com/canonical/chisel/internal/archive_test [github.com/canonical/chisel/internal/archive.test]
internal/archive/archive_test.go:48:22: undefined: archive.FakeDo
FAIL    github.com/canonical/chisel/internal/archive [build failed]
FAIL
woky commented 2 years ago

Sorry, I switched the umask. It's actually 002 in Ubuntu and 022 in ArchLinux. You can reproduce with this:

$ go test ./internal/deb
ok      github.com/canonical/chisel/internal/deb    0.045s
$ umask 022
$ go clean -testcache
$ go test ./internal/deb

----------------------------------------------------------------------
FAIL: extract_test.go:285: S.TestExtract

Test: Extract nothing
[LOG] 0:00.000 Extracting files from package "base-files"...
Test: Extract a few entries
[LOG] 0:00.002 Extracting files from package "base-files"...
extract_test.go:311:
    c.Assert(result, DeepEquals, test.result)
... obtained map[string]string = map[string]string{"/etc/":"dir 0755", "/etc/os-release":"symlink ../usr/lib/os-release", "/tmp/":"dir 01755", "/usr/":"dir 0755", "/usr/bin/":"dir 0755", "/usr/bin/hello":"file 0755 eaf29575", "/usr/lib/":"dir 0755", "/usr/lib/os-release":"file 0644 ec6fae43", "/usr/share/":"dir 0755", "/usr/share/doc/":"dir 0755"}
... expected map[string]string = map[string]string{"/etc/":"dir 0755", "/etc/os-release":"symlink ../usr/lib/os-release", "/tmp/":"dir 01775", "/usr/":"dir 0755", "/usr/bin/":"dir 0755", "/usr/bin/hello":"file 0775 eaf29575", "/usr/lib/":"dir 0755", "/usr/lib/os-release":"file 0644 ec6fae43", "/usr/share/":"dir 0755", "/usr/share/doc/":"dir 0755"}
... Difference:
...     ["/tmp/"]: "dir 01755" != "dir 01775"
...     ["/usr/bin/hello"]: "file 0755 eaf29575" != "file 0775 eaf29575"

OOPS: 3 passed, 1 FAILED
--- FAIL: Test (0.01s)
FAIL
FAIL    github.com/canonical/chisel/internal/deb    0.008s
FAIL
niemeyer commented 2 years ago

The problem is with the test.

So let's fix the test. The point I've been trying to make is that we don't want to change the environment of the actual code being tested to fix tests, as otherwise we might be hiding a real bug that needs to be fixed. We should instead create the tested files with the actual correct permissions. There's logic already there to do that inside the implementation, for instance.

woky commented 2 years ago

@niemeyer You are right. I'm going to close this one and open another one.