go test cache: include umask as a test input for caching?

[twp...@gmail.com]

tl;dr: umask is a system-wide global that affects go tests and should be considered when validating go test's cache.

Motivation:

I'm the author of a popular open source project that writes files to the user's home directory and cares about getting exact file permissions correct. The file permissions depend on the the current umask setting. As umask is a process global variable, it cannot be set for individual tests, and so separate tests are needed for different umasks.

Currently changing the umask does not invalidate go test's cache, so I get incorrect test results if I change the umask and re-run go test.

Suggested solution:

Include umask as an op in go test's id calculation.

Next steps:

* Is this something that the Go project would consider?

* If so, I would be happy to submit a CL.

Thanks,

Tom

[Ian Lance Taylor]

Personally, I would approach this kind of thing by writing a test that
sets the umask to various different values and invokes subtests with
each umask value. That way the test is independent of the external
environment.

In general our approach has been that if your test intrinsically
depends on the external environment, then you should run it with
-test.run=1 to disable the test cache.

Ian

[twp...@gmail.com]

> Personally, I would approach this kind of thing by writing a test that
sets the umask to various different values and invokes subtests with
each umask value.

I would love to do this but umask is a process global (as far as I understand it) and so can't be set within subtests.

[Jason Phillips]

Why can't it be set within subtests? Note that subtests (like regular tests) aren't run in parallel unless you explicitly call t.Parallel().

[twp...@gmail.com]

umask cannot be set in subtests for two reasons:

1. It is a process-wide global variable stored by the operating system. Changing the umask in a test changes the umask for the entire process, i.e. it changes the umask for all tests.

2. It is not possible to set and restore the umask atomically. This makes it inherently racy for concurrent programs.

To learn more about umask, and why it is special, please read the man page.

[Kurtis Rader]

You wrote earlier:

> Currently changing the umask does not invalidate go test's cache, so I get incorrect test results if I change the umask and re-run go test

I interpret that to mean you are changing the umask before running `go test`. That is, you might run `go test` with different umask values and don't want the cached results to ignore the umask value. As did Ian if I correctly understood their reply. The discussion then evolved into how a specific unit test can safely change the umask without affecting other unit tests. Running unit tests with different umask values is a different problem from how to write unit tests that modify the umask value. At this point it is unclear, at least to me, what problem you are trying to solve.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/c9dcf1bb-25ae-4e58-8714-66325077c2d1n%40googlegroups.com.

--

Kurtis Rader

Caretaker of the exceptional canines Junior and Hank

[twp...@gmail.com]

> I interpret that to mean you are changing the umask before running `go test`. That is, you might run `go test` with different umask values and don't want the cached results to ignore the umask value. As did Ian if I correctly understood their reply. The discussion then evolved into how a specific unit test can safely change the umask without affecting other unit tests. Running unit tests with different umask values is a different problem from how to write unit tests that modify the umask value.

That is correct. Unfortunately there seems to be a lack of knowledge in some cases of what umask is and how it behaves, which is why the discussion got sidetracked.

> At this point it is unclear, at least to me, what problem you are trying to solve.

I want go test to invalidate its cache when the umask is changed, i.e. I want the following sequence of commands to work correctly:

$ umask 002

$ go test ./...

$ umask 022

$ go test ./... # should not use cache

$ go test ./... # should use cache

Currently, the go test cache does not record the umask, so it incorrectly re-uses the cached test results in the second invocation of go test. It should instead invalidate the cache.

Note that go test does record the external files used by the tests and the external environment variables used by the tests, and so it correctly invalidates the cache when any of them change. I would like the cache to also be invalidated when the external umask changes. Alternately put, go test already has special case code for external changes to files and environment variables, and I would like extend that to external umask changes too.

Hope this makes my request clearer.

I'd be delighted to submit at CL to do this (it's probably 10-20 lines of code with almost no performance penalty [1]), but I want to check that it has a reasonable chance to being accepted before I start the work.

[1] The performance cost is two very fast syscalls during startup.

[Axel Wagner]

On Tue, 17 Sept 2024 at 10:33, twp...@gmail.com <twp...@gmail.com> wrote:

umask cannot be set in subtests for two reasons:

1. It is a process-wide global variable stored by the operating system. Changing the umask in a test changes the umask for the entire process, i.e. it changes the umask for all tests.

2. It is not possible to set and restore the umask atomically. This makes it inherently racy for concurrent programs.

I might be misunderstanding something, but neither of these seems to actually be a problem. Tests are not run concurrently, unless explicitly requested by calling `t.Parallel()`. The same goes for sub tests, I believe. So, simply doing

func TestOne(t *testing.T) {

    defer syscall.Umask(syscall.Umask(0))

    // test goes here

}

func TestTwo(t *testing.T) {

    defer syscall.Umask(syscall.Umask(0777))

    // test goes here

}

Should work, AIUI, even with the issues you mention. Or is there something else going on that I'm unaware of?

--

[twp...@gmail.com]

> Should work, AIUI, even with the issues you mention. Or is there something else going on that I'm unaware of?

The problem is not running the tests. The problem is that the test cache is not invalidated correctly.

On Tuesday, September 17, 2024 at 11:52:35 AM UTC+2 Axel Wagner wrote:

On Tue, 17 Sept 2024 at 10:33, twp...@gmail.com <twp...@gmail.com> wrote:

umask cannot be set in subtests for two reasons:

1. It is a process-wide global variable stored by the operating system. Changing the umask in a test changes the umask for the entire process, i.e. it changes the umask for all tests.

2. It is not possible to set and restore the umask atomically. This makes it inherently racy for concurrent programs.

I might be misunderstanding something, but neither of these seems to actually be a problem. Tests are not run concurrently, unless explicitly requested by calling `t.Parallel()`. The same goes for sub tests, I believe. So, simply doing

func TestOne(t *testing.T) {

    defer syscall.Umask(syscall.Umask(0))

    // test goes here

}

func TestTwo(t *testing.T) {

    defer syscall.Umask(syscall.Umask(0777))

    // test goes here

}

[Stephen Illingworth]

If the only problem is that the cache is getting in the way then you can run the tests such that test cache is not used. The idiomatic way is to use -count=1 on the test command line

I would do this:

go test -count=1 <umask tests>

And then:

go test <other tests>

[twp...@gmail.com]

On Tuesday, September 17, 2024 at 12:44:42 PM UTC+2 Stephen Illingworth wrote:

If the only problem is that the cache is getting in the way then you can run the tests such that test cache is not used. The idiomatic way is to use -count=1 on the test command line

I would do this:

go test -count=1 <umask tests>

And then:

go test <other tests>

I am aware that I can work around go test cache's incorrect behavior by disabling the cache. This is annoying for several reasons:

* I have to manually maintain a list of tests that are affected by the umask.

* I have to run go test twice.

* I don't get the speed-ups of caching when the caching should work, which slows down my development cycle.

Note that all tests that write files are affected by umask, even if many people don't realize this. To demonstrate this, find a project that calls testing.T.TempDir() and run:

$ umask 777

$ go test ./... -count=1

The odds are that the tests will fail.

However, I want to make go test cache's behavior more correct for a case where it is currently incorrect for no measurable performance penalty. What are the reasons for *not* doing this?

[Axel Wagner]

I think you are probably going to get better results by just filing a proposal. That's the normal process which will end in a definite result one way or the other, while mailing list threads tend to go in circles, eventually.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/27617075-a38b-4e01-8412-b290aab3f906n%40googlegroups.com.

[Ian Lance Taylor]

On Tue, Sep 17, 2024 at 3:26 AM twp...@gmail.com <twp...@gmail.com> wrote:
>
> > Should work, AIUI, even with the issues you mention. Or is there something else going on that I'm unaware of?
>
> The problem is not running the tests. The problem is that the test cache is not invalidated correctly.

We are suggesting that you change the umask in the test itself.

This is perfectly reliable as tests are not run in parallel (unless
you explicitly call t.Parallel). The umask will not affect any code
other than the test code itself.

You can see an example of this in the Go standard library:
https://go.googlesource.com/go/+/refs/heads/master/src/os/os_unix_test.go#231

Ian

[Ian Lance Taylor]

On Tue, Sep 17, 2024 at 4:08 AM twp...@gmail.com <twp...@gmail.com> wrote:
>
> However, I want to make go test cache's behavior more correct for a case where it is currently incorrect for no measurable performance penalty. What are the reasons for *not* doing this?

We do not want to record all aspects of the external environment that
could possibly affect the test. That would be too much. So we need
some way to know what should be recorded and what should not. What
should that be? Our current approach is simple: we record environment
variables that the test reads and we record files opened within the
module but not files opened outside of the module. Note that if a
test opens some file outside of the module, we cache the test results
even if that file changes.

What approach should we use for test caching that will tell us that
umask should be cached?

Ian