Refactor tests and CI #96

alexandrevicenzi · 2024-09-18T12:53:30Z

Use github.com/stretchr/testify to simplify test assertions
Rewrite a few tests that failed to run as root (e.g. inside the Golang container)
Rewrite CI jobs to be a bit more clear about what is being tested
Run unit tests inside the BCI Golang container

Signed-off-by: Alexandre Vicenzi <[email protected]>

When running tests inside a container they often run as root. Signed-off-by: Alexandre Vicenzi <[email protected]>

Create helper scripts to run checks as this allows to run checks locally easily. Signed-off-by: Alexandre Vicenzi <[email protected]>

dirkmueller · 2024-09-30T07:57:23Z

internal/regionsrv/hostsfile_test.go

@@ -53,6 +53,12 @@ func TestUpdateHostsFileCouldNotRead(t *testing.T) {
 }

 func TestUpdateHostsFileCouldNotWrite(t *testing.T) {
+	// TODO: is there a better way to test this?
+	//       root can read/write anything
+	if os.Getuid() == 0 {


I wonder if we shouldn't simply have a test that fails if os.Geteuid() returns 0 rather than hacking on the tests to pass as root.

I personally would never run a unittest suite as root. I am not sure we should go down the path of supporting that, especially when it means that some of the tests are skipped then. that basically means the testing coverage is not the same. and that's bad.

Honestly, in a perfect scenario, the test case should not care about which user is being run. I find it completely wrong to assume in a test that a given path is not readable, it can be readable for certain users.

Some tests IMHO did not make much sense the way they were written. Code coverage is one thing, the usefulness of a test is another.

Many tests have an assumption that things can't be read just because the user has no permission, but container-suseconnect runs as a zypper plugin, which I believe runs as root, and in theory has permissions for everything.

This project is rather small. testing error paths is important for customer resiliency. unit tests should be testing error and edge conditions, as they tend to break during refactorings. It'd be fine to use mocking to reliably trigger specific error conditions and test the code that way. without that, documenting the sensible assumption of somebody not running unittest code as root would be imho a better way than skipping tests when you run it anyhow as root. it makes the assumption explicit rather than hiding it under the rug.

there can be other reasons why a file could not be written to, even as root, so the condition is valid to test I think.

dirkmueller · 2024-09-30T08:00:08Z

build/ci/formatting

+echo "* Checking code styling *"
+echo "*************************"
+
+go install mvdan.cc/gofumpt@latest


it was intentionally not installed previously so that you can use it from a packaged source rather go install it unconditionally on your system. ..

What do you suggest here? Simply fail the test if the package is not installed?

The package can be installed in the ci.yaml file before the execution, so it would be available for CI.

if it's already installed, use the installed version. otherwise fail it or skip it imho. BTW, I'm not sure we need to use a 3rd party style checker when the standard one is good enough. what qualities do we gain by adding another dependency?

dirkmueller · 2024-09-30T08:01:45Z

I'm not sure I understand what is easier with the CI scripts being broken out into individual shell scripts rather than having a make file which you can invoke with make test and it does it all though? I would like to be able to preserve the ability to invoke them as part of the package builds, so the ci jobs now, that require networking unconditionally would have to be fixed to be runnable without network again.

dirkmueller · 2024-09-30T08:03:23Z

internal/credentials_test.go

-	}
+	assert.Contains(t, locs, "/etc/zypp/credentials.d/SCCcredentials")
+	assert.Contains(t, locs, "/run/secrets/SCCcredentials")
+	assert.Contains(t, locs, "/run/secrets/credentials.d/SCCcredentials")


this isn't the same. previously it was checking for a specific ordering of the locations, now you are accepting it at any order. given that lookup order matters, why are we not checking on locs[0] with assertequal instead?

If the order is important, then the test should check it accordingly. I though that the order was not that critical.

the previous test was testing ordering

the ordering is important to validate that configs are read in the right order (so that first or last one wins, whatever is implemented)

dirkmueller

generally +1 on the move to testify library, seems like it cleans up the code a lot. there's one inline comment to address though. I'm a bit skeptical on the usefulness of the ci job refactoring, see inline comments.

alexandrevicenzi · 2024-09-30T11:41:44Z

I'm not sure I understand what is easier with the CI scripts being broken out into individual shell scripts rather than having a make file which you can invoke with make test and it does it all though? I would like to be able to preserve the ability to invoke them as part of the package builds, so the ci jobs now, that require networking unconditionally would have to be fixed to be runnable without network again.

I don't see a good reason for makefiles in a Go project, but I could keep it and have a make test and call the scripts if that makes it easier for others.

Splitting into multiple scripts makes it easier to understand which checks we run in the CI and also allows us to split jobs a bit more easily, before that, there was one big job, which is not ideal. With multiple jobs I can easily see which check failed without looking at any CI action logs.

Having multiple scrips also allows in the future to enforce a few checks (e.g. go fmt) with pre-commit hooks, this is something I consider adding.

The network requirement can be removed, it is not mandatory to install any packages, we can simply fail (or skip) if not installed. That was an oversight, I should have remembered that we can potentially run these scripts in OBS.

dirkmueller · 2024-10-09T06:29:29Z

I don't see a good reason for makefiles in a Go project, but I could keep it and have a make test and call the scripts if that makes it easier for others.

makefiles in go projects are very idiomatic, and provide a number of benefits:

they can automate tedious steps to type and thereby reduce the risk of human error. with a makefile, we can do perform just the steps that are necessary, but also always the steps that are necessary in the right order without the risk of oversights in between.
provide consistency: it ensures that the project is tested and built in the same way, regardless of who is working on it or on which system it is being built
incremental builds: makefiles can skip steps that are time consuming and unnecessary because nothing it depends on from a previous run has changed.

Splitting into multiple scripts makes it easier to understand which checks we run in the CI and also allows us to split jobs a bit more easily, before that, there was one big job, which is not ideal. With multiple jobs I can easily see which check failed without looking at any CI action logs.

that's fair, but that can be done with a makefile as well (have multiple targets).

Having multiple scrips also allows in the future to enforce a few checks (e.g. go fmt) with pre-commit hooks, this is something I consider adding.

Same can be done with a script calling the makefile with a specific target.

The network requirement can be removed, it is not mandatory to install any packages, we can simply fail (or skip) if not installed. That was an oversight, I should have remembered that we can potentially run these scripts in OBS.

Thanks!

alexandrevicenzi added 2 commits September 18, 2024 12:53

Refactor tests to use testify library

5fc08ea

Signed-off-by: Alexandre Vicenzi <[email protected]>

Allow tests to run as root

713299a

When running tests inside a container they often run as root. Signed-off-by: Alexandre Vicenzi <[email protected]>

alexandrevicenzi requested review from dirkmueller and dcermak September 18, 2024 12:54

Improve CI jobs

702645b

Create helper scripts to run checks as this allows to run checks locally easily. Signed-off-by: Alexandre Vicenzi <[email protected]>

alexandrevicenzi force-pushed the testing branch from 7407ed6 to 702645b Compare September 18, 2024 12:58

dirkmueller reviewed Sep 30, 2024

View reviewed changes

dirkmueller requested changes Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor tests and CI #96

Refactor tests and CI #96

alexandrevicenzi commented Sep 18, 2024

dirkmueller Sep 30, 2024

alexandrevicenzi Sep 30, 2024

dirkmueller Oct 9, 2024

dirkmueller Sep 30, 2024

alexandrevicenzi Sep 30, 2024

dirkmueller Oct 9, 2024

dirkmueller commented Sep 30, 2024

dirkmueller Sep 30, 2024

alexandrevicenzi Sep 30, 2024

dirkmueller Oct 9, 2024

dirkmueller left a comment

alexandrevicenzi commented Sep 30, 2024

dirkmueller commented Oct 9, 2024 •

edited

Loading

Refactor tests and CI #96

Are you sure you want to change the base?

Refactor tests and CI #96

Conversation

alexandrevicenzi commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirkmueller commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirkmueller left a comment

Choose a reason for hiding this comment

alexandrevicenzi commented Sep 30, 2024

dirkmueller commented Oct 9, 2024 • edited Loading

dirkmueller commented Oct 9, 2024 •

edited

Loading