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

Unit tests for resource Get requests #412

Merged
merged 3 commits into from
Jan 11, 2021

Conversation

gauravgahlot
Copy link
Contributor

Description

The PR adds unit tests for Get requests to resources (template, hardware, workflow).
Restructures the tests in #361 as per the new framework.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@gauravgahlot gauravgahlot self-assigned this Jan 8, 2021
@codecov
Copy link

codecov bot commented Jan 8, 2021

Codecov Report

Merging #412 (44073a7) into master (57eb0ef) will increase coverage by 3.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   29.90%   33.04%   +3.13%     
==========================================
  Files          24       24              
  Lines        2170     2170              
==========================================
+ Hits          649      717      +68     
+ Misses       1445     1377      -68     
  Partials       76       76              
Impacted Files Coverage Δ
db/template.go 48.46% <0.00%> (+0.76%) ⬆️
db/workflow.go 29.18% <0.00%> (+2.65%) ⬆️
db/db.go 45.45% <0.00%> (+9.09%) ⬆️
db/hardware.go 77.98% <0.00%> (+47.70%) ⬆️

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 57eb0ef...44073a7. Read the comment docs.

var wg sync.WaitGroup
wg.Add(len(s.Input))
for _, hw := range s.Input {
if s.InputAsync {
Copy link
Contributor

Choose a reason for hiding this comment

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

You are using InputAsync with a GetByID? Can you rephrase it? It does not have a lot of sense for me

}
}(ctx, tinkDB, hw)
} else {
wg.Done()
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall I don't understand what you are doing. You should check what you expect as part of the table test. Not here

wg.Add(len(s.Input))
for _, in := range s.Input {
if s.InputAsync {
go func(t *testing.T, ctx context.Context, tt *workflow.Workflow, tinkDB *db.TinkDB) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before, it does not have sense to use InputAsync here because you are not doing any input.

@@ -211,6 +204,192 @@ func TestDeleteHardware(t *testing.T) {
}
}

type getFunc func(c context.Context, db *db.TinkDB, in *hardware.Hardware) (string, error)

func TestGetHardware(t *testing.T) {
Copy link
Contributor

@gianarb gianarb Jan 8, 2021

Choose a reason for hiding this comment

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

You are testing different functions in the same test and that's not good practice, please split the functions you are testing in their own test case (keep table testing as well) thanks

@gauravgahlot gauravgahlot force-pushed the db-get-tests branch 2 times, most recently from 0899b8f to 8cd6bae Compare January 11, 2021 06:34
Signed-off-by: Gaurav Gahlot <[email protected]>
Signed-off-by: Gaurav Gahlot <[email protected]>
@gauravgahlot gauravgahlot force-pushed the db-get-tests branch 2 times, most recently from c72be29 to c0f1118 Compare January 11, 2021 07:27
@gauravgahlot gauravgahlot marked this pull request as ready for review January 11, 2021 07:33
}
}

func TestGetWorkflow_WithNonExistingID(t *testing.T) {
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 move this test here TestGetWorkflow? it is just another variant of the table test.

Signed-off-by: Gaurav Gahlot <[email protected]>
@gianarb gianarb added the ready-to-merge Signal to Mergify to merge the PR. label Jan 11, 2021
@mergify mergify bot merged commit 1f1ee52 into tinkerbell:master Jan 11, 2021
@gauravgahlot gauravgahlot deleted the db-get-tests branch January 12, 2021 05:56
@gauravgahlot gauravgahlot mentioned this pull request Jan 12, 2021
3 tasks
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants