-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Adds many more static workflow parser tests #30
Adds many more static workflow parser tests #30
Conversation
See #28 - the conditional operations will soon become less complicated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very thorough, LGTM!
|
||
describe('getNodeInfo', () => { | ||
it('Returns nodeInfo containing only \'unknown\' nodeType if nodeId is undefined', () => { | ||
const nodeInfo = getNodeInfo(newWorkflow(), undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we distinguish between if a node was not found, or only its type wasn't? I can see if the returned value has an unknown node type, we log an error saying so, but perhaps this is misleading if the node doesn't exist, or the graph isn't defined.. etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I switched it to returning unknown only when a matching nodeID is found, but the type isn't step or DAG. All of the other cases where the graph or nodeID are invalid return nulls
expect(nodeInfo.containerInfo!.inputs).toEqual([['param1', ''], ['param2', '']]); | ||
}); | ||
|
||
it('Returns nodeInfo containing container outputs as list of name/value duples, pulling from valueFrom if necessary', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/duples/tuples/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops. I didn't realize that that was just a musical term
]); | ||
}); | ||
|
||
it('Returns empty strings for outputs with no specified value', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the same test on 422?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is outputs, the other's inputs. The logic ends up being the same, but it's done by different lines of code.
expect(nodeInfo.stepsInfo).toEqual({ conditional: '{{someVar}} == something', parameters: [[]]); | ||
}); | ||
|
||
it('Returns nodeInfo with step template\'s arguments when node template has \'when\' property', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"does not have 'when' property"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I think I also need to |
... hmmm and I guess |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yebrahim The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments addressed in #45
]); | ||
}); | ||
|
||
it('Returns empty strings for outputs with no specified value', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is outputs, the other's inputs. The logic ends up being the same, but it's done by different lines of code.
expect(nodeInfo.containerInfo!.inputs).toEqual([['param1', ''], ['param2', '']]); | ||
}); | ||
|
||
it('Returns nodeInfo containing container outputs as list of name/value duples, pulling from valueFrom if necessary', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops. I didn't realize that that was just a musical term
expect(nodeInfo.stepsInfo).toEqual({ conditional: '{{someVar}} == something', parameters: [[]]); | ||
}); | ||
|
||
it('Returns nodeInfo with step template\'s arguments when node template has \'when\' property', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
describe('getNodeInfo', () => { | ||
it('Returns nodeInfo containing only \'unknown\' nodeType if nodeId is undefined', () => { | ||
const nodeInfo = getNodeInfo(newWorkflow(), undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I switched it to returning unknown only when a matching nodeID is found, but the type isn't step or DAG. All of the other cases where the graph or nodeID are invalid return nulls
…ing (kubeflow#31) * We were checking the wrong variable in the loop. * This was a typo introduced when we switched to supporting multiple workflows. * Fix kubeflow#30
run_e2e_workflow.py is exiting with an exception causing all prow jobs to be marked as a failure Add an E2E test to kubeflow/testing. This provides a way to test run_e2e_workflow.py as part of the presubmits. Fix kubeflow#30
Updating directory in Dockerfile
UPSTREAM:<carry>:fix:Endpoint fix for AWS S3 Bucket Session
This change is