[#21384] Increase sql package test coverage from 35% to 70% #33711
Conversation
|
Assigning reviewers. If you would like to opt out of this review, comment R: @lostluck for label go. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
dev-mohit06
changed the title
|
@lostluck Can you please review and merge it to the repo? |
There was a problem hiding this comment.
Thank you very much Mohit! This is wonderful. I do have some style and consistency comments however. It's a good rule of thumb to not introduce a wildly different style to a codebase within a given language, as it makes it harder to work in.
What you have here is good, but it can be better (though you might need to do a bit of reading first).
| @Use: This test ensures that the OutputType option behaves as expected | ||
| when used in SQL transformations. | ||
| */ |
There was a problem hiding this comment.
In Go, we use // for doc comment blocks, not /* */. In particular, multi-line comments are generally used sparingly, such as it would make a copy paste of some command simpler. Please change this for consistency with the rest of the project.
Please also remove the @ blocked lines, they either repeat the initial test description, or don't add anything. It's also not the Apache Way to take credit for any given block of code in the files themselves. That is sufficiently tracked via this commit existing attached to your Github profile, and the resulting blame lines pointing to it once it is merged in.
The same applies to all blocks below.
See https://google.github.io/styleguide/go/decisions#package-comments generally for package comments.
| typ := reflect.TypeOf(int64(0)) | ||
| components := []typex.FullType{typex.New(reflect.TypeOf(""))} | ||
|
|
||
| // Test without components |
There was a problem hiding this comment.
In go, prefer using Table Tests instead of single test bodies with comments for each test case.
See https://google.github.io/styleguide/go/decisions#table-driven-tests specifically, and https://google.github.io/styleguide/go/decisions#test-structure more generally.
| opt1 := OutputType(typ) | ||
| opt1(o) | ||
| expected1 := typex.New(typ) | ||
| if !reflect.DeepEqual(o.outType, expected1) { |
There was a problem hiding this comment.
Prefer to use cmp.Equal or cmp.Diff instead of reflect.DeepEqual. Consider printing the diff, which will be a clearer description of the issue.
See https://google.github.io/styleguide/go/decisions#print-diffs
|
|
||
| // Verify all fields | ||
| if _, ok := o.inputs[name]; !ok { | ||
| t.Error("Input option not applied correctly") |
There was a problem hiding this comment.
Test output is much better when the input, both the received and the expected outputs are included in the test message.
"Input option with name %q, not applied correctly: got %v, want %v", name, got, want"
And similar.
See also https://mtlynch.io/if-got-want-improve-go-tests/ which is an excellent approach for this sort of thing too.
|
@lostluck, thank you for telling me. As I am learning Go, I will improve myself and make the changes you mentioned above. |
PR Summary:
This PR addresses #21384 by adding comprehensive unit tests to the
sqlpackage, increasing the code coverage from 35% to 75%. The following test cases have been added:Testing:
Checklist:
CHANGES.mdAdditional Notes:
TestTransformtests validate the required options without needing the expansion service.Screenshots:
Coverage Output:
Coverage.out File:
Test Cases Passing Locally:
