This repository has been archived by the owner on May 31, 2024. It is now read-only.
fix: Converts WDL log exit_code from string to int32 per WES spec #380
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
OhCloud
force-pushed
the
ohCloud_ExitCodeConversion
branch
from
b23c939
to
0a4257e
Compare
elliot-smith
previously approved these changes
Mar 23, 2022
OhCloud
force-pushed
the
ohCloud_ExitCodeConversion
branch
from
4638966
to
0a4257e
Compare
OhCloud
force-pushed
the
ohCloud_ExitCodeConversion
branch
from
5dbe853
to
0a4257e
Compare
markjschreiber
suggested changes
Mar 23, 2022
| s.Assert().Equal(testTaskJobId, tasks[0].JobId) | ||
| s.Assert().True(tasks[0].StartTime.Equal(testStartTime.Truncate(time.Second))) | ||
| s.Assert().True(tasks[0].StopTime.Equal(testStopTime.Truncate(time.Second))) | ||
| s.Assert().Empty(nil, "NA") |
There was a problem hiding this comment.
This tests if nil is "empty" which it always is. Perhaps you meant to test that ExitCode is empty?
There was a problem hiding this comment.
awesome catch, thanks Mark! i've updated the test.
| }, nil) | ||
|
|
||
| tasks, err := s.manager.GetWorkflowTasks(testRunId) | ||
| if s.Assert().NoError(err) { |
There was a problem hiding this comment.
Please remove the if statement as the if isn't needed but the assert is (same as the test below).
| @@ -200,6 +201,58 @@ func (s *GetWorkflowTasksTestSuite) TestGetWorkflowTasks_WithTaskNameFailure() { | |||
| } | |||
| } | |||
|
|
|||
| func (s *GetWorkflowTasksTestSuite) TestGetWorkflowTasks_WithExitCodeUnknown() { | |||
There was a problem hiding this comment.
NIT: Please update this to "TestGetWorkflowTasks_WithExitCodeNil"
OhCloud
force-pushed
the
ohCloud_ExitCodeConversion
branch
from
a420299
to
aeac892
Compare
elliot-smith
approved these changes
Mar 24, 2022
4 tasks
|
@markjschreiber Are you satisfied with the changes here? I want to do my revision of #317 based on top of this, since I'm going to need it for that feature to actually work, and there will be much less git surgery for me to do later if this is merged. |
markjschreiber
approved these changes
Mar 30, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Issue #, if available: #314
Description of Changes
Per the wes spec AGC was incorrectly requiring a string instead of an int. This fix includes updates/returns the
exit_code's string to the requiredint32or"NA"for both successful/unsuccessful cases.Description of how you validated changes
I have added additional unit tests validating a successful and unsuccessful cases. Additionally, this was validated in the terminal.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
d