Add test coverage to pkg/term#33520
Conversation
hack/make/test-unit
Outdated
pkg/term/term_linux_test.go
Outdated
There was a problem hiding this comment.
The first tty will get closed twice, because the defer acts on the initial value of tty. The second one doesn't get closed.
Signed-off-by: Naveed Jamil <naveed.jamil@tenpearls.com>
|
LGTM ping @tiborvass |
|
LGTM |
| var rootEnabled bool | ||
|
|
||
| func init() { | ||
| flag.BoolVar(&rootEnabled, "test.root", false, "enable tests that require root") |
There was a problem hiding this comment.
Why is this flag necessary? Why not just skip the tests if the uid isn't 0?
There was a problem hiding this comment.
I suppose that would work, but I think I prefer the flag, because it means the set of tests that runs won't be sensitive to the environment. It's nice to avoid scenarios where the tests pass for one person but fail for another person running the same comment.
containerd uses this flag to enable/disable tests that require root privileges.
There was a problem hiding this comment.
I'm more concerned that tests don't run unless you happen to provide this flag. and there's really no way to discover this flag. I really don't think this is a good solution. Skipping if uid != 0 is the same behaviour without the need to discover a flag.
If we know a test is going to fail, why bother running it? This is what we do for all the integration tests, we skip them if the environment does not support the test. This is pretty standard for many tests suites I think.
If for some reason we really want to force these to run, unless someone sets something explicit then I think we should be using an environment variable. We can always run the tests, unless the environment variable PKG_TERM_SKIP_ROOT_ONLY_TESTS is set. Then we can inform users about this env var with an error message when uid != 0.
| state, err := SetRawTerminal(tty.Fd()) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, state) | ||
| err = DisableEcho(tty.Fd(), state) |
There was a problem hiding this comment.
I'm running the unit tests on master and I'm seeing my shell is missing echo after running these tests. I think this tests needs to re-enable echo to clean up after itself.
I'm preparing a PR to fix this now.
Signed-off-by: Naveed Jamil naveed.jamil@tenpearls.com
- What I did
Increased test coverage for pkg/term.
- How to verify it
Run
go test