tests: increase coverage of store.go

[jrusz]

I am creating this now just to get some specific advice, I know I need to do better naming, cover some more functions and probably some more changes.

I recently talked with @teg about how to tackle this after some recent changes and so what I intend to do here is create a temporary directory and in there create a new state.json (Line:17-18) and fill it with data using all the different methods in store.go and use that to check the functionality of the other methods that retrieve the data and more... I want to delete the temp directory once all the tests are finished. I could do that by just deferring the deletion of the dir but that means I would have to do all the tests inside one function which I don't really like, I like it more when each function/method has it's own test function. So what I did now is created the temp dir as a global variable which seems to work fine but I have to remember to delete it at some point probably in the last function (Line:74), but I don't think this is correct because the dir would not get removed in case of some error and the function where it's being removed would not be ran.

Any thoughts what's the best approach to solve this @teg @atodorov ?

[schutzbot]

I love to test code, but an admin needs to say ok to test before I can get started. 😛

[ondrejbudai]

TestMain is your friend here: https://medium.com/goingogo/why-use-testmain-for-testing-in-go-dafb52b406bc

[jrusz]

@ondrejbudai Thanks a lot! That works perfectly :)

[teg]

Rather than use global variables, I would create a new dir/store for each test that you want to do. Otherwise, this looks like a good start to me.

[jrusz]

Ok I believe it's in a state worthy of review. This is just testing the functions that are working with Blueprints. I want to get that merged before moving on.

[ondrejbudai]

If I understand this correctly, these tests are dependant on each other and also on their order. I could not find a reliable source, but several SO posts[1][2] speak against depending on the test order.

I suggest encapsulating all the code in one big test and using the subTests, which does have a well-defined order. Also, using this method we can get rid of all the global variables,

[1] https://stackoverflow.com/questions/31201858/how-to-run-golang-tests-sequentially
[2] https://stackoverflow.com/questions/30171693/execute-test-cases-in-a-pre-defined-order

[atodorov]

several SO posts[1][2] speak against depending on the test order.

Indeed having inter dependencies between tests is generally considered bad practice. Each test must setup/teardown around itself. If this is undesirable (e.g. too expensive) then all of them should use a common environment but not be dependent on the state of the environment. If there are specific pre-conditions then each test must ensure they are met before doing any testing.

FTR some test runners (e.g. rspec) can execute tests in randomized order which help discover and fix these inter dependencies. To my knowledge most other languages don't provide this ability.

[jrusz]

I tried to use the TestWrapper approach with a few tests now just to see how it looks and I find this to be pretty much the same like having one big test case. On the outside I just see

=== RUN   TestWrapper
--- PASS: TestWrapper (0.00s)
PASS
coverage: 18.4% of statements
ok      github.com/osbuild/osbuild-composer/internal/store      1.015s  coverage: 18.4% of statement

I liked it more when I could see all the test cases but as I understand it you can't do that when using a wrapper. But I guess making sure the tests run in the order I want is more important here.

[jrusz]

Indeed having inter dependencies between tests is generally considered bad practice. Each test must setup/teardown around itself. If this is undesirable (e.g. too expensive) then all of them should use a common environment but not be dependent on the state of the environment.

Ok I understand it's better to have each test case independent but here it's easier to just share that one state and assert the functions with that.

If there are specific pre-conditions then each test must ensure they are met before doing any testing.

If the order in which the tests are being run ensures that and I ensure the order is always the same then it's ok?

EDIT:
I realized that that's not good enough either, dependencies are indeed bad, even if I ensure the test execution order some tests will still fail if the other test it was dependent on failed. That will create a cascade of failures because of just one failure. I'm going to make each one independent.

[jrusz]

So I tried another approach using testify/suite I think it looks pretty nice, setup and teardown looks easy, but the tests are executed in alphabetical order right now and I guess that could change in the future so I should not rely on that.. Some discussion was held here.
Also the output looks like this:

=== RUN   TestExampleTestSuite
=== RUN   TestExampleTestSuite/TestA_NewEmpty
=== RUN   TestExampleTestSuite/TestB_PushBlueprint
=== RUN   TestExampleTestSuite/TestC_ListBlueprints
=== RUN   TestExampleTestSuite/TestD_PushBlueprintToWorkspace
--- PASS: TestExampleTestSuite (0.01s)
    --- PASS: TestExampleTestSuite/TestA_NewEmpty (0.00s)
    --- PASS: TestExampleTestSuite/TestB_PushBlueprint (0.00s)
    --- PASS: TestExampleTestSuite/TestC_ListBlueprints (0.00s)
    --- PASS: TestExampleTestSuite/TestD_PushBlueprintToWorkspace (0.00s)
PASS
coverage: 18.4% of statements
ok      github.com/osbuild/osbuild-composer/internal/store      1.021s  coverage: 18.4% of statements

[ondrejbudai]

It seems to be that testify/suite is just syntactic sugar for go subtests. See https://blog.golang.org/subtests

[atodorov]

Ok I understand it's better to have each test case independent but here it's easier to just share that one state and assert the functions with that.

This is a unit test and such coupling between the test functions and order of execution is a very bad practice IMO. This generally makes the test suite/test functions more fragile.

It could be acceptable for an integration style test which needs expensive setup operations but that doesn't seem to be the case here.

The suite approach looks better to me, also that's what you can generally see in other tests written in go (e.g. blogs, examples, etc).

I still don't understand what necessitates the inter-dependencies between test functions and their order of execution? In particular which test functions depend on what state or which other test functions? Answering this should help with untangling the functions from one another.

Why not have the setup/teardown as functions and just call them before/after each test ? Will also solve the issue with os.Exit and defer.

[jrusz]

@atodorov You're right, I want to avoid bad practices and learn to do whatever is best. I like the testify/suite approach quite a bit. I just pushed a commit where I have a setup function that sets up my variables that I need and then a setup and teardown that gets run before and after each test case as well. It was not a big change after all and now each test is independent. I just need to check if I still have the same coverage, figure out better naming and clean up commented code.

Also the output looks like this now:

=== RUN   TestExampleTestSuite
=== RUN   TestExampleTestSuite/TestDeleteBlueprint
=== RUN   TestExampleTestSuite/TestDeleteBlueprintFromWorkspace
=== RUN   TestExampleTestSuite/TestGetBlueprint
=== RUN   TestExampleTestSuite/TestGetBlueprintChange
=== RUN   TestExampleTestSuite/TestGetBlueprintChanges
=== RUN   TestExampleTestSuite/TestGetBlueprintCommited
=== RUN   TestExampleTestSuite/TestListBlueprints
=== RUN   TestExampleTestSuite/TestPushBlueprint
=== RUN   TestExampleTestSuite/TestPushBlueprintToWorkspace
=== RUN   TestExampleTestSuite/TestRandomSHA1String
=== RUN   TestExampleTestSuite/TestTagBlueprint
=== RUN   TestExampleTestSuite/Test_NewEmpty
--- PASS: TestExampleTestSuite (0.02s)
    --- PASS: TestExampleTestSuite/TestDeleteBlueprint (0.00s)
    --- PASS: TestExampleTestSuite/TestDeleteBlueprintFromWorkspace (0.00s)
    --- PASS: TestExampleTestSuite/TestGetBlueprint (0.00s)
    --- PASS: TestExampleTestSuite/TestGetBlueprintChange (0.00s)
    --- PASS: TestExampleTestSuite/TestGetBlueprintChanges (0.00s)
    --- PASS: TestExampleTestSuite/TestGetBlueprintCommited (0.00s)
    --- PASS: TestExampleTestSuite/TestListBlueprints (0.00s)
    --- PASS: TestExampleTestSuite/TestPushBlueprint (0.00s)
    --- PASS: TestExampleTestSuite/TestPushBlueprintToWorkspace (0.00s)
    --- PASS: TestExampleTestSuite/TestRandomSHA1String (0.00s)
    --- PASS: TestExampleTestSuite/TestTagBlueprint (0.00s)
    --- PASS: TestExampleTestSuite/Test_NewEmpty (0.00s)
PASS
coverage: 35.0% of statements
ok      github.com/osbuild/osbuild-composer/internal/store      1.027s  coverage: 35.0% of statements

[atodorov]

LGTM

[ondrejbudai]

I like it, thanks!

This introduces a new library to the tests. I wonder if we should decide on a usage policy. I propose a simple "use it when you feel it's appropriate". Tagging @teg, @larskarlitski and @msehnout, so they know about this change.

[ondrejbudai]

Imports are not grouped properly (no big issue)

[jrusz]

Oh, do we have some rules for this or are there any common rules for this? I always just rely on go fmt to do all this work.

[ondrejbudai]

We try sorting the imports like this:

1. standard library
2. 3rd party packages
3. our packages

However, we don't enforce it (there's sadly no way to enforce it on the CI).

[jrusz]

Now that I know this, I can enforce it on me myself :D ... Is it correct now?

[atodorov]

Can prepare-source / go fmt take care of this somehow ?

[atodorov]

However, we don't enforce it (there's sadly no way to enforce it on the CI).

@ondrejbudai there must be linters for that, right? At least in Python both pylint (and I think) isort follow the same policy as you described above.

[ondrejbudai]

There's goimports. I tried it and it had very strange results in some cases.

Then there's impi, but I wasn't brave enough to use it, it doesn't seem much popular. However, I think it does the grouping right. Feel free to investigate it if you see a value in it.

[larskarlitski]

This introduces a new library to the tests. I wonder if we should decide on a usage policy. I propose a simple "use it when you feel it's appropriate". Tagging @teg, @larskarlitski and @msehnout, so they know about this change.

Sounds good to me. Thanks for letting me know.

[teg]

Sounds good to me too!

[teg]

Thanks for this!