Athens: Ensure storage tests can run without intermittent failures

[arschles]

Storage tests right now run in parallel against the same shared datastore, so intermittent failures can happen due to race conditions (@marwan-at-work mentioned that he's seen some).

We should think about whether we can do something to make this more "foolproof". One idea is to use testify/suite's SetupTest and TearDownTest functions to provide each test with its own database (or whatever is appropriate for the given storage driver)

Another option is to run with go test -p 1 ... to ensure _all_ tests don't run in parallel (this is what the buffalo CLI does). I don't feel like that's an ideal solution, but it works for them.

cc/ @michalpristas @robjloranger @marwan-at-work since we've all talked about testing in one way or another recently

[marwan-at-work]

@arschles should this issue apply to all storages and not just RDMBS?

In any case, I think each test can get an "acquire" and "clean" functions as arguments so that each test can have its own namespace inside the Storage and a way to clean up afterwards.

This not only allows running tests in parallel, but running the tests again on the same DB instance.

PS. for RDBMS, seems like we're on the path to remove the whole storage anyway: https://github.com/gomods/athens/issues/423#issuecomment-412160092?

[michalpristas]

i like -p 1 solution from Buffalo. I'm not a fan of tweaking code just to make tests work.

[marwan-at-work]

@michalpristas if you run storage tests twice in a row, they fail. Because the DB doesn't get cleaned up after the first time, is that ok? If so, we should at least document that "you can only run the tests once, otherwise you have to clean the DB manually".

[michalpristas]

yes, I agree with the cleanup at the end of the tests, this is a must as well.
But if tests operate on the same collection we should keep them running sequentially

[fedepaol]

Can I work on this? Are we talking about the tests for pkg/storage right?

[arschles]

@fedepaol absolutely. And yes, we're talking primarily about pkg/storage tests, but this also applies to tests in cmd/proxy/actions because they use the storage drivers in pkg/storage

[fedepaol]

OMG, my first officially assigned issue!

[arschles]

yessss! 💯

[fedepaol]

Need some help.

If I look at https://github.com/gomods/athens/blob/master/pkg/storage/storage_tests/module_storage/storage_test.go#L71 that's the only test function, so even if we enable running the tests in parallel, all the storage tests are there. Which means that the storage tests are called sequentially

Moreover, each storage has its own datastore (like mongo, fs, etc). What am I missing?

[robjloranger]

I think mostly you need to make sure each test suite cleans up after itself. One of the issues is that an artifact from a previous test could remain to interfere with a subsequent test.

[fedepaol]

I gave a most thorough look, and tried to run the storage tests more than once.

• the tests run _sequentially_ as per my previous comment. Since the only entry point is TestStorage, they can't run in parallel and I am not sure where to find the race conditions mentioned by @arschles
• every test calls store.Cleanup() which seem to be implemented properly
• rdbms tests did not have Cleanup() implemented (see https://github.com/gomods/athens/blob/90281ccb908a5414126d6e948d4b68f23a4725f7/pkg/storage/rdbms/test_suite.go#L36 )
• I ran the tests more than once not incurring in the problem

Said that, are we sure there is something to fix here? I have the feeling that by removing the rdbms support everything else is working fine now (at least, looking at storage_tests )

[manugupt1]

@fedepaol We have removed RDBMS package from master. Can you verify if that is the case again? I checked it again and I did not find it.

[fedepaol]

@manugupt1 what I meant here is that before we had tests for rdbms with no cleanup, which might have been the cause of this issue.

I linked an old revision to show that. Now that they are gone with the rdbms package, this issue could not be a problem anymore.

[manugupt1]

@fedepaol Ah okay! So, this might be fixed!

[devdinu]

We should cleanup, but even after we do that, might hit into race condition. while cleaning we can't drop table, but clear its own records.

we should have records different for each storage and cases. eg: storage_name_operation_module, if we've same module with save and list cleanup might happen before other completes.

[fedepaol]

@devdinu Not sure race conditions are the problem. As I wrote before, there is only one test function that runs all the tests sequentially, one storage at the time, which is https://github.com/gomods/athens/blob/master/pkg/storage/storage_tests/module_storage/storage_test.go#L71

[marpio]

@fedepaol is there anything to do here or can we close it?

[fedepaol]

@marpio I think this can be closed