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

Refactoring and test fixes. #70

Merged
merged 2 commits into from
Aug 6, 2018

Conversation

Skarlso
Copy link
Member

@Skarlso Skarlso commented Aug 5, 2018

This PR fixes the following:

  • When tests are sharing services they must clean up after themselves or the next test will use an already initialised service. This means that things like, the location of the database might be cached, or if the service is mocked out, it might not work with the next test. Therefor a defer func() { services.MockStorageService(nil)}() is needed for cleanup.
  • Some tests are deleting the database file. The services though are only initialised if the underlying service singleton is nil. If not, it will just return that, but will fail never the less because the file doesn't exists like the db, or the certificate. Again, a set to nil will fix this problem.
  • os.TempDir isn't unique enough. There are cases now that the tests step on each other. ioutil.TempDir("", "TestName") must be used to ensure semi uniqueness. Maybe add a random number in there.
  • Minimal change to use an even smaller repository in tests which only contains fmt instead of the whole world as dependency which made the test slow and clunky.

There might be more cases when running a test something doesn't work. A good first step is add in a defer mock to nil for services that the code might use. Like, vault, or storage.

Also, pro tip... once in a while run go clean -testcache! This will erase every cached content and force tests to run. Might surprise you that some tests start to fail even though you though they are passing. Watch out for this tid-bit: ok github.com/gaia-pipeline/gaia/handlers **(cached)**.

@Skarlso Skarlso requested a review from michelvocks August 5, 2018 20:17
@codecov-io
Copy link

codecov-io commented Aug 5, 2018

Codecov Report

Merging #70 into master will increase coverage by 0.94%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   60.03%   60.98%   +0.94%     
==========================================
  Files          18       18              
  Lines        1544     1548       +4     
==========================================
+ Hits          927      944      +17     
+ Misses        510      500      -10     
+ Partials      107      104       -3
Impacted Files Coverage Δ
services/service_provider.go 77.77% <100%> (+35.55%) ⬆️
security/vault.go 76.69% <100%> (+0.17%) ⬆️
pipeline/git.go 61.11% <60.71%> (+0.19%) ⬆️
security/ca.go 72.41% <0%> (-1.73%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81d8f41...59305b2. Read the comment docs.

@Skarlso Skarlso added the Ready To Merge PR is ready to be merged into master label Aug 5, 2018
@Skarlso
Copy link
Member Author

Skarlso commented Aug 5, 2018

Ready to Merge, of course only after review. :)

Copy link
Member

@michelvocks michelvocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks really good to me. Good job and thanks a lot! @Skarlso 🤗

I also think about having one helper function that sets up gaia.Cfgwhich we can simply call on every test start up. What do you think?

Edit: We can add that btw. in another PR at another time 😄

@@ -4,7 +4,7 @@ GO_LDFLAGS_STATIC=-ldflags "-s -w -extldflags -static"
default: dev

dev:
go run ./cmd/gaia/main.go -homepath=${PWD}/tmp -dev=true -poll=true
go run ./cmd/gaia/main.go -homepath=${PWD}/tmp -dev=true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we already talked about this. I think we can leave it in. What do you think? 🤗

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will just merge it in now. We can add it later again if decision has been made 🤗

@michelvocks michelvocks merged commit 52d975c into gaia-pipeline:master Aug 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready To Merge PR is ready to be merged into master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants