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

[WIP] test: run root test with build flag #500

Closed
wants to merge 1 commit into from

Conversation

IronCore864
Copy link
Contributor

@IronCore864 IronCore864 commented Sep 19, 2024

5 tests moved to build flag

  • internals/daemon: TestUserGroup
  • internals/daemon: TestUserIDGroupID
  • internals/daemon: TestWriteUserGroupReal
  • internals/daemon: TestMakeDirsUserGroupReal
  • internals/osutil: TestMakeParentsChmodAndChown

TestUserGroup and TestUserIDGroupID are moved into a new file internals/daemon/api_exec_root_test.go, and since they require some setup/teardown, I created a TestMain func there. TestWriteUserGroupReal and TestMakeDirsUserGroupReal are moved into a separate new file internals/daemon/api_files_root_test.go, they don't need setup, but because it's in the same package, there is a problem: the TestMain from the other file still runs for these tests. The tests won't fail, but doing a non-required setup feels wrong.

The only solution I can think of is to create a new folder, something like the integration tests and put TestUserGroup and TestUserIDGroupID in one subfolder/package, and put TestWriteUserGroupReal and TestMakeDirsUserGroupReal into a different subfolder/package, so that the TestMain func from one won't interfere with the other. Any better solution here?

1 test not moved to build flag:

  • internals/overlord/servstate: TestUserGroup

This is because the setup/teardown for this test is complicated, moving it to another new file basically means copy-pasting most parts of the existing setup/teardown code and rewriting them not using a suite. I did part of this and found it was too complicated so I gave it up and reverted the changes. Need some input here about this test.

@benhoyt
Copy link
Contributor

benhoyt commented Sep 19, 2024

Hmmm, actually looking at this now, it'd be nice to keep all the tests in the place they belong.

I wonder if we can solve this a much simpler way. When I just run the tests as root with go test ./..., it actually works (and runs the root tests fine):

$ sudo su
$ PEBBLE_TEST_USER=ben PEBBLE_TEST_GROUP=ben go test ./...

There are a few failures from tests that expect a permissions failure or something, and don't get the failure as root, so the test fails, for example:

FAIL: cmd_add_test.go:28: PebbleSuite.TestAdd

cmd_add_test.go:103:
    c.Assert(os.IsPermission(err), check.Equals, true)
... obtained bool = false
... expected bool = true

However, those would be easy to tweak, or even c.Skip if os.Getuid() == 0.

Then we could just run the root tests with a single command like this:

PEBBLE_TEST_USER=runner PEBBLE_TEST_GROUP=runner sudo -E go test ./...

@IronCore864
Copy link
Contributor Author

Closing this PR according to the above discussion. We will run all tests with root so that all tests can be kept together in the same place where the package is.

@IronCore864 IronCore864 deleted the root-test-poc branch September 22, 2024 09:54
IronCore864 added a commit that referenced this pull request Sep 23, 2024
Background: Previously, when adding a new root test, we had to modify
the GitHub Actions workflow to include the newly added test, which is
some extra operational overhead.

Per discussion
[here](#500 (comment)),
we agreed to run all tests with root and skip those tests that would
fail with root (checking permissions and stuff), and this PR skips tests
that would fail when running with root and made the GitHub Actions
workflow for root tests much easier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants