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.

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

tests/cmd/snapctl: unset SNAP_CONTEXT for the suite #7843

Conversation

anonymouse64
Copy link
Member

@anonymouse64 anonymouse64 commented Dec 4, 2019

This test specifically would fail when go was run from a snap, because snap-confine for the go snap would set SNAP_COOKIE from here:

// for compatibility, if facing older snapd.
setenv("SNAP_CONTEXT", snap_context, 1);

which would then cause snapctl to pick up the SNAP_CONTEXT variable and not see that the SNAP_COOKIE variable is unset and show a help message. So if we unset this environment variable when running this test suite, we are protected against this possibility.

For reference, the test would fail like this:

----------------------------------------------------------------------
FAIL: main_test.go:108: snapctlSuite.TestSnapctlHelp

main_test.go:61:
    c.Assert(snapctlOptions.ContextID, Equals, s.expectedContextID)
... obtained string = "dFPq0qs83PLy2tHK5YmFVss3v1V6yVGvjwDhwp1H80yD"
... expected string = ""

main_test.go:117:
    c.Check(err, IsNil)
... value client.ConnectionError = client.ConnectionError{Err:(*url.Error)(0xc0000206c0)} ("cannot communicate with server: Post http://127.0.0.1:34175/v2/snapctl: EOF")

OOPS: 2 passed, 1 FAILED
--- FAIL: TestT (0.05s)
FAIL

This variable can also be used to specify the cookie to use for snapctl, and
sometimes other tests can leak this env var, so to be safe just unset it in the
start of our test to ensure that we always use SNAP_COOKIE for this test

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
@anonymouse64 anonymouse64 added the Simple 😃 A small PR which can be reviewed quickly label Dec 4, 2019
@@ -47,6 +47,8 @@ var _ = Suite(&snapctlSuite{})

func (s *snapctlSuite) SetUpTest(c *C) {
os.Setenv("SNAP_COOKIE", "snap-context-test")
// don't use SNAP_CONTEXT, in case other tests accidentally leak this
os.Unsetenv("SNAP_CONTEXT")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks fine, it shouldn't be a problem if we unset it in the test binary.

Still, I don't see it being set anywhere in the package. AFAIK, when running go test ./..., each package would get built and run separately, so even if a test in another package sets SNAP_CONTEXT, this should not leak into the test in another package. Is it possible that the tests were executed by Go installed from a snap?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it's weird, it would be nice to know where did it come from.

That being said, the change looks ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it possible that the tests were executed by Go installed from a snap?

Yes this is exactly what I did, for some reason it doesn't happen when I just use ./run-checks --unit instead

Still, I don't see it being set anywhere in the package

FWIW it is still set by snap-confine here:

// for compatibility, if facing older snapd.
setenv("SNAP_CONTEXT", snap_context, 1);

Copy link
Contributor

@stolowski stolowski left a comment

Choose a reason for hiding this comment

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

+1

@@ -47,6 +47,8 @@ var _ = Suite(&snapctlSuite{})

func (s *snapctlSuite) SetUpTest(c *C) {
os.Setenv("SNAP_COOKIE", "snap-context-test")
// don't use SNAP_CONTEXT, in case other tests accidentally leak this
os.Unsetenv("SNAP_CONTEXT")
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, it's weird, it would be nice to know where did it come from.

That being said, the change looks ok.

@anonymouse64
Copy link
Member Author

anonymouse64 commented Dec 5, 2019

So after looking into it some more, the issue with this test is that I was running

$ go test ./...

instead of

$ ./run-checks --unit

and the former is go 1.13 and the latter will sneakily find my go 1.10 install in /usr/lib/go-1.10/bin:

snapd/run-checks

Lines 19 to 22 in 4c562a1

if [ -z "${TRAVIS_BUILD_ID:-}" ]; then
# when *not* running inside travis, ensure we use go-1.10 by default
export PATH=/usr/lib/go-1.10/bin:${PATH}
fi

Why this test fails on go 1.13 and passes on go 1.10 is a bit of a mystery to me however.

@anonymouse64
Copy link
Member Author

Actually this is because my go 1.10 is not from a snap and go 1.13 is, if I use go 1.10 from a snap it still fails:

$ go_110 version
go version go1.10.8 linux/amd64
$ go_110 test  github.com/snapcore/snapd/cmd/snapctl

----------------------------------------------------------------------
FAIL: main_test.go:108: snapctlSuite.TestSnapctlHelp

main_test.go:61:
    c.Assert(snapctlOptions.ContextID, Equals, s.expectedContextID)
... obtained string = "GxY3qsx529070VkFQyWwJ4bbVd1cdMvdqtX5WgqLjXst"
... expected string = ""

main_test.go:116:
    c.Check(err, IsNil)
... value client.ConnectionError = client.ConnectionError{Err:(*url.Error)(0xc4202b4000)} ("cannot communicate with server: Post http://127.0.0.1:34895/v2/snapctl: EOF")

OOPS: 2 passed, 1 FAILED
--- FAIL: TestT (0.01s)
FAIL
FAIL    github.com/snapcore/snapd/cmd/snapctl   0.013s

@anonymouse64 anonymouse64 merged commit eeff8f7 into snapcore:master Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Simple 😃 A small PR which can be reviewed quickly
Projects
None yet
3 participants