Start adding unit tests #1
Conversation
kavir1698
requested review from
sbliven and
minottic
and removed request for
sbliven
There was a problem hiding this comment.
in general, I think all changes which are not covered by tests shouldn't be here, and be part of dedicated PRs potentially with tests.
I try to keep clean history with git rebase, rather than having merge commits. This means that commits of PR, when merged, appear as commits on main without the merge one. For this, I would suggest that every commit should pass tests (so that we can apply the concept of "every commit on main works"). To achieve this, either we squash the commits together on merge (which means that the squashed commit should be conceptually "groupable") or you can interactively rebase commit together before merge.
Last, it's usually useful to have some "rules" when writing commits: https://cbea.ms/git-commit/
| // "flag" | ||
| ) | ||
|
|
||
| // TestMainHelp is a test function that verifies the output of the main function. |
There was a problem hiding this comment.
general comment: thanks a lot for the comments, they are very useful! I think after some time, we can simply discard them, as otherwise the text will become a little too dense, especially if they are on third-party or native go functions
There was a problem hiding this comment.
Yes, eventually the comments could be reduced.
cmd/datasetIngestor/main_test.go
Outdated
| } | ||
| } | ||
|
|
||
| // Errors with the test above. |
There was a problem hiding this comment.
what's this part? I'd rather remove it than have it commented, we can add it later.
|
|
||
| // Restore stdout after the test | ||
| // defer is a keyword that schedules a function call to be run after the function that contains the defer statement has completed. | ||
| defer func() { |
There was a problem hiding this comment.
I saw golang has setup/tear down test syntax: https://pkg.go.dev/testing#hdr-Main. Maybe this is fine like it is for now though...
cmd/datasetIngestor/main_test.go
Outdated
| "bytes" | ||
| "os" | ||
| "testing" | ||
| // "flag" |
cmd/datasetIngestor/main_test.go
Outdated
| // if isFlagPassed("undefinedflag") { | ||
| // t.Errorf("isFlagPassed() succeeded for flag that was not defined") | ||
| // } | ||
| // } |
There was a problem hiding this comment.
missing newline. Maybe consider adding some linting in the CI, so this will fail the tests. https://golangci-lint.run/ (as a side comment, I think the CI addition should have been a PR)
There was a problem hiding this comment.
I think such things would fail the tests. It didn't fail here because it was a comment.
cmd/datasetRetriever/main.go
Outdated
There was a problem hiding this comment.
I think all these changes shouldn't be here, as they are not covered by tests and we should be able to revert if they fail. So they should be in another PR, potentially with dedicated tests when you'll tackle this component.
There was a problem hiding this comment.
These changes cannot be removed or the tests fail. although they are not directly checked for these tests, they are simply wrong code that causes the tests to fail.
datasetUtils/getUserInfoFromToken.go
Outdated
There was a problem hiding this comment.
I think all these changes shouldn't be here, as they are not covered by tests and we should be able to revert if they fail. So they should be in another PR, potentially with dedicated tests when you'll tackle this component.
| // TestMainHelp is a test function that verifies the output of the main function. | ||
| // It captures the stdout, runs the main function, and checks if the output contains the expected strings. | ||
| // This just checks if the main function prints the help message. | ||
| func TestMainHelp(t *testing.T) { |
There was a problem hiding this comment.
food for thought: I think this could be simplified mocking fmt.Printf and asserting that this is called with "\n\nTool to ingest datasets to the data catalog.\n\n". https://stackoverflow.com/questions/52315410/mocking-go-methods. This could be some googling though, so happy to keep it as is!
This commit adds a simple test for datasetIngestor main.go. It also fixes a few bugs in the code that prevented the test to pass.