test: add unit tests for engine exec #180
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
reviewpad-bot
added
large
shay2025
force-pushed
the
test/engine-exec
branch
from
248aea4 to
3c115a2
Compare
shay2025
force-pushed
the
test/engine-exec
branch
from
f51945b to
cc4ac84
Compare
shay2025
force-pushed
the
test/engine-exec
branch
2 times, most recently
from
d050eca to
ef91b7c
Compare
ferreiratiago
requested changes
Aug 3, 2022
There was a problem hiding this comment.
I would suggest to refactor the test and have into consideration table testing. Here's a starting point:
func TestEval_Labels() {
"when label has no name"
"when label exists"
"when label does not exist"
"when label is invalid"
}
func TestEval_Groups() {
"when group is valid"
"when group is invalid"
}
func TestEval_Rules() {
"when rule is valid"
"when rule is invalid"
}
func TestEval_Workflows() {
"when workflow is invalid" // e.g. rule is invalid
"when no workflow is activated"
"when one workflow is activated"
"when multiple workflows are activated"
"when workflow should always run"
"when workflow is activated and has extra actions"
}
You can check hasUnaddressedReviewThreads_test.go to have an idea.
Also, I would suggest to have a different reviewpad.yml file for each test case in testdata/exec. For instance:
- reviewpad_with_unmaned_label.yml
- reviewpad_with_valid_label.yml (this can be used for
"when label does not exist"and"when label exists") - ...
- reviewpad_with_one_worfklow
- reviewpad_with_multiple_worfklows
WDYT?
shay2025
force-pushed
the
test/engine-exec
branch
from
ef91b7c to
ebea37c
Compare
shay2025
force-pushed
the
test/engine-exec
branch
from
e051f1c to
d444062
Compare
ferreiratiago
requested changes
Aug 8, 2022
utils/file/file.go
Outdated
Comment on lines
14
to
35
| func LoadReviewpadFile(filepath string) ([]byte, error) { | ||
| reviewpadFileData, err := os.ReadFile(filepath) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return reviewpadFileData, nil | ||
| } | ||
|
|
||
| func ParseReviewpadFile(data []byte) (*engine.ReviewpadFile, error) { | ||
| reviewpadFile := &engine.ReviewpadFile{} | ||
| err := yaml.Unmarshal(data, &reviewpadFile) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // At the end of loading all imports from the file, its imports are reset to []engine.PadImport{}. | ||
| // However, the parsing of the wanted reviewpad file, sets the imports to []engine.PadImport(nil). | ||
| reviewpadFile.Imports = []engine.PadImport{} | ||
|
|
||
| return reviewpadFile, nil | ||
| } |
There was a problem hiding this comment.
Please review these functions.
shay2025
force-pushed
the
test/engine-exec
branch
from
d1f4d40 to
74656c6
Compare
ferreiratiago
approved these changes
Aug 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request introduces the unit tests for engine exec and some extra mocks related to the pull request data.
During the development of the unit tests, it was encountered an error in the
CollectErrorfunction where it wasn't being taken into account the*github.ErrorResponseerror type and this would cause an nil pointer dereference because theError()method would be applied to this object which doesn't support such thing and so we need to manually get the error string in this case.Related issue
Closes #176
Type of change
Bug fix (non-breaking change which fixes an issue)
Improvements (non-breaking change without functionality)
How was this tested?
Ran
task testcmd.Checklist
task check -fand have no issues