-
Notifications
You must be signed in to change notification settings - Fork 173
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
Define end-to-end test for version endpoint [specific ci=Group23-Vic-Machine-Service] #6475
Define end-to-end test for version endpoint [specific ci=Group23-Vic-Machine-Service] #6475
Conversation
5511407
to
75bbbc0
Compare
1aedfcd
to
2c72e45
Compare
${auth}= Evaluate base64.b64encode("%{TEST_USERNAME}:%{TEST_PASSWORD}") modules=base64 | ||
${RC} ${OUTPUT}= Run And Return Rc And Output curl -s -w "\n\%{http_code}\n" -X GET "http://127.0.0.1:${HTTP_PORT}/container/target/%{TEST_URL}/${PATH}?thumbprint=%{TEST_THUMBPRINT}" -H "Accept: application/json" -H "Authorization: Basic ${auth}" | ||
${STATUS}= Get Line ${OUTPUT} -1 | ||
Set Test Variable ${RC} |
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.
I haven't seen this pattern before, but I think I like it. It makes the tests very clean.
${auth}= Evaluate base64.b64encode("%{TEST_USERNAME}:%{TEST_PASSWORD}") modules=base64 | ||
${RC} ${OUTPUT}= Run And Return Rc And Output curl -s -w "\n\%{http_code}\n" -X GET "http://127.0.0.1:${HTTP_PORT}/container/target/%{TEST_URL}/${PATH}?thumbprint=%{TEST_THUMBPRINT}" -H "Accept: application/json" -H "Authorization: Basic ${auth}" | ||
${STATUS}= Get Line ${OUTPUT} -1 | ||
Set Test Variable ${RC} |
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.
I wouldn't suggest having these keywords in a separate file as I think it will be run as test suite as its under test suite dir but @mhagen-vmware can confirm that. Also, I don't like the corresponding .md
file.
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.
I wouldn't suggest having these keywords in a separate file as I think it will be run as test suite as its under test suite dir but @mhagen-vmware can confirm that.
If it gets run as a suite, it seems like it will get run as a suite with zero tests. That isn't a problem locally. Is it a problem for our CI infrastructure?
(Replying to @mhagen-vmware here, to keep related conversation in a single thread.)
this should be a resource file within our resources folder and needs to be added into resources/Util.robot please
These keywords aren't intended to be used by other test groups. Defining them in the resources folder and adding that file to resources/Util.robot
essentially adds them to a global namespace.
Do we have a better pattern for sharing keywords between suites within a single test group?
Is there a functional problem with this pattern, or is the complaint only that it's stylistically different than what we've done in the past?
I don't like the corresponding
.md
file.
I don't like it either, but was attempting to following the formatting conventions. I'd rather use that file as documentation for these keywords, but I think I'd need to format it differently. Are we okay with that?
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.
Is it a problem for our CI infrastructure?
No, but that pattern (having util files under tests) doesn't seem right to me.
Are we okay with that?
I would suggest removing it for now as we don't have any documentation for existing keywords.
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.
No, but that pattern (having util files under tests) doesn't seem right to me.
I moved the resource file so that it's under resources, and named it in a way that makes it clear it's intended for use within this test group.
I would suggest removing it for now as we don't have any documentation for existing keywords.
I removed the corresponding md file.
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.
yea, this is really not using any of our existing patterns.. this should be a resource file within our resources folder and needs to be added into resources/Util.robot please
404f76e
to
5998081
Compare
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.
few suggestions which i think will make this pattern better.
@@ -0,0 +1,59 @@ | |||
# Copyright 2017 VMware, Inc. All Rights Reserved. |
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.
Can we rename this file to something more general, may be like VIC-Machine-Service-Util.robot
? We may use it in other test suites as well.
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.
I don't want to give the file a more general name because I don't want these keywords used in other test groups.
Our current process allows for only two types of keywords: suite-scoped and global. This file follows a different pattern: group-scoped.
Defining all shared keywords in the global scope carries many of the same drawbacks as defining variables in the global scope: identifying relevant keywords becomes harder, determining where keywords are used becomes harder, safely changing the behavior of keywords becomes harder, concisely and accurately naming keywords becomes harder, etc.
We'll iterate on this design as we learn more about this pattern, but I think this is a reasonable starting point.
Set Test Variable ${STATUS} | ||
|
||
|
||
Verify Return Code |
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.
I would put all these verify keywords in a separate util file, may be, called Verify-Util.robot
.
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.
Given the close dependency between the way the Get
(and eventually Post
/Put
/Delete
) methods store results and the way the Verify
methods make assertions, I think it's important to keep the keywords in the same file; using the Verify
keywords without the Get
keywords wouldn't make sense.
Start Process ./bin/vic-machine-server --port ${HTTP_PORT} --scheme http shell=True cwd=/go/src/github.com/vmware/vic | ||
|
||
|
||
Get Path |
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.
As this is in global space now, can we rename this keyword to little more specific?
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.
Placing these keywords in the global scope would, as you suggest, require providing more specific names for the keywords. However, within the scope of this test group I think the keyword names are clear.
Set Test Variable ${STATUS} | ||
|
||
|
||
Get Path Under Target |
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.
As this is in global space now, can we rename this keyword to little more specific?
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.
As above.
448a3c8
to
c2d649a
Compare
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.
@zjs I'm fine with iterating this further later, would suggest adding an issue for that.
Good idea! Filed #6508. |
c2d649a
to
45d4f6b
Compare
Introduce a single-test robot suite for the version endpoint as well as a utility resource for use by this suite and others within the group.