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

Setup integration test suite for db package #387

Merged
merged 6 commits into from
Dec 16, 2020
Merged

Setup integration test suite for db package #387

merged 6 commits into from
Dec 16, 2020

Conversation

gianarb
Copy link
Contributor

@gianarb gianarb commented Dec 11, 2020

Description

TestContainer is a library that spins up and down containers programmatically inside a test case. It is very comfortable to run integration tests and to set up requirements such as Postgres in our case.

This PR contains the first tests that validate the happy path for the CreateTemplate function.

Question

Right now in order to separate the tests requiring docker from the one who can run standalone I created a separate package ./db/_integration that by default is not picked up by go test ./...

The thread off here is that code coverage does not count the new package, even if it will be nice to have it included. A possible solution is to add a flag like:

go test --with-docker ./...

if the flag is set the tests requiring docker will run. In this way, we should have code coverage counting integration tests.

Why is this needed

Because it is better to refactor a package when it is covered by tests.

@gianarb gianarb marked this pull request as draft December 11, 2020 09:55
@codecov
Copy link

codecov bot commented Dec 11, 2020

Codecov Report

Merging #387 (b780f41) into master (266637a) will decrease coverage by 7.88%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
- Coverage   28.71%   20.82%   -7.89%     
==========================================
  Files          19       24       +5     
  Lines        1386     2170     +784     
==========================================
+ Hits          398      452      +54     
- Misses        961     1678     +717     
- Partials       27       40      +13     
Impacted Files Coverage Δ
workflow/template_validator.go 91.66% <75.00%> (-3.34%) ⬇️
db/template.go 34.61% <100.00%> (ø)
db/db.go 9.09% <0.00%> (ø)
db/events.go 0.00% <0.00%> (ø)
db/hardware.go 0.00% <0.00%> (ø)
db/workflow.go 0.20% <0.00%> (ø)

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 266637a...b780f41. Read the comment docs.

@gianarb gianarb added the ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/ label Dec 11, 2020
Gianluca Arbezzano added 3 commits December 14, 2020 09:52
This commit bootstrap a possible integration test suite for the db pkg.

It uses TestContainers to create an manage container lifecycle as code
inside a test case.

In order to run those tests you need Docker enable, for this reason you
have to execute them explicitly:

  $ go test -v ./db/_integration
  === RUN   TestCreateTemplate
  === RUN   TestCreateTemplate/happy-path-single-crete-template
  === PAUSE TestCreateTemplate/happy-path-single-crete-template
  === CONT  TestCreateTemplate/happy-path-single-crete-template
  2020/12/11 10:43:03 Starting container id: 0ea745bc8539 image: quay.io/testcontainers/ryuk:0.2.3
  2020/12/11 10:43:03 Waiting for container id 0ea745bc8539 image: quay.io/testcontainers/ryuk:0.2.3
  2020/12/11 10:43:03 Container is ready id: 0ea745bc8539 image: quay.io/testcontainers/ryuk:0.2.3
  2020/12/11 10:43:03 Starting container id: a61bdbf3b7c3 image: postgres:13.1
  2020/12/11 10:43:03 Waiting for container id a61bdbf3b7c3 image: postgres:13.1
  2020/12/11 10:43:04 Container is ready id: a61bdbf3b7c3 image: postgres:13.1
      db_test.go:64: applyed 5 migrations
  --- PASS: TestCreateTemplate (0.00s)
      --- PASS: TestCreateTemplate/happy-path-single-crete-template (2.42s)
  PASS
  ok  	github.com/tinkerbell/tink/db/_integration	2.676s

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
This commit add the integration tests as part of the CI checks

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
This commit adds more tests coming from #361 only for templates

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
Comment on lines 57 to 64
CHECK_DB:
for ii := 0; ii < 5; ii++ {
err = dbCon.Ping()
if err != nil {
t.Log(errors.Wrap(err, "db check"))
time.Sleep(1 * time.Second)
goto CHECK_DB
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this kind of doesn't make any sense right? If not ready go back to before the loop forever. If it is ready... make sure its ready 5 times?

Copy link
Contributor

@mmlb mmlb left a comment

Choose a reason for hiding this comment

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

lgtm so far. Lets just set the tests up to run by default and call t.Skip() if it detects that docker is not available.

ContainerRequest: testcontainers.ContainerRequest{
Image: "postgres:13.1",
ExposedPorts: []string{"5432/tcp"},
WaitingFor: wait.ForLog("database system is ready to accept connections"),
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add support to wait for something like wait.Exec and then we can just call pg_isready.

@@ -31,6 +33,26 @@ func Parse(yamlContent []byte) (*Workflow, error) {
return &workflow, nil
}

// MustParse parse a slice of bytes to a template. It an error occurs the
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add tests for these so codecov is happy?

mmlb
mmlb previously approved these changes Dec 16, 2020
db/db_test.go Show resolved Hide resolved
I moved the test back to the db package as suggested by Manny and now
tests automatically skip if Docker is not running.

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
@gianarb gianarb marked this pull request as ready for review December 16, 2020 16:17
@gianarb gianarb requested a review from mmlb December 16, 2020 16:17
This commit tests the two new functions introduced as utilities for
template and workflow testing

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
@gianarb gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Dec 16, 2020
@mergify mergify bot merged commit c4d73cc into tinkerbell:master Dec 16, 2020
@gianarb gianarb deleted the chore/bootstrap-integration-test-for-dbpkg branch December 16, 2020 16:43
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-check/vagrant-setup This label trigger a GitHub action that tests the Vagrant Setup guide https://tinkerbell.org/setup/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants