-
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
Add basic integration tests for VCH inspect API #7290
Conversation
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.
Also some bug fixes in API delete:
As the delete changes are logically separate from the inspect test you've written, my personal preference would be to push the delete fixes as a separate commit, which might require manually squashing the other commits and fixing up the commit messages before doing a "Rebase and merge" of the PR. Not sure if that's actually possible with our repository configuration though. ¯\_(ツ)_/¯
|
||
Verify VCH Inspect Output | ||
# debug | ||
Property Should Be Equal .debug 1 |
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.
nit: this might be slightly more readable if you insert extra spaces between .debug
and 1
so that 1
is vertically aligned with the other values, below
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.
ah, missed this one. thanks
|
||
# version | ||
${version}= Get Version String | ||
Property Should Be Equal .version ${version} |
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.
nit: as above, I think this would be more readable if .version
were vertically aligned with the other properties and ${version}
were vertically aligned with the other values
|
||
Property Should Contain .storage.image_stores[0] %{TEST_DATASTORE} | ||
Property Should Contain .storage.volume_stores[0].datastore %{TEST_DATASTORE}/%{VCH-NAME}-VOL | ||
Property Should Be Equal .storage.volume_stores[0].label default |
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 also be verifying that .image_stores
and .volume_stores
contain only a single element? (We verify that the first element of each is what we're expecting, but the test would still pass if there were other bogus values showing up in the response.)
Solving this elegantly may require adding a new Property Should ...
keyword (perhaps along the lines of Property Should Be Unary
, which we could use for each array we expect to have a single element, or perhaps along the lines of Property Length Should Be
, which we could use for each array we expect to have a specific length).
Get Path Under Target Using Session datacenter/${DC-ID}/vch/${VCH-ID} | ||
|
||
Verify VCH Inspect Output | ||
# debug |
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.
Could we also verify the other top-level properties, like .name
?
@zjs Yes you're right, it's better to put the delete bug fixes in a separate pr. Idk what's the best way to do it but I could always cherry-pick commits to a new branch, and manually revert back this branch...not sure if it will work...let me try and see update: did |
0860e88
to
24a04f8
Compare
I recall us testing that volumes in existing volume stores still exist after a reinstall as part of the delete tests. This behaviour should have been caught there if it's a product code issues, or we need a keyword that expects existing volume stores during VCH install. The flip side of that is that the regular VCH install keyword should confirm that volume stores don't exist and error if they do - that way we're being explicit about our state expectations for the test. |
Yes, we have a test case that re-installs VCH using existing volume stores, and then checks after installation completes that the volume stores exist. But the re-install keyword does a cleanup on test server first, which could accidentally delete the existing volume stores.
The reason why this behavior hasn't been caught on CI is that because CI checks drone status and skips cleanup if the test is running (but when I ran locally, it failed, and that's how I found out this issue). But yes passing |
Kind of blocked by this ticket: #7321 |
If that issue blocks this PR, we should go ahead and fix it. (It may only be P2 on its own merits, but if it's blocking a P1 we should treat it as a P1.) |
cleanup=${false} was added for nightly tests, because some tests don't have VCH installed anymore at the end and resulted in a 5 minute hang trying to gather logs from a non-existent VCH. |
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.
Just discovered this comment in pending from a long time back. Still seems relevent.
@@ -78,6 +78,11 @@ Delete Path Under Target | |||
Set Test Variable ${OUTPUT} | |||
Set Test Variable ${STATUS} | |||
|
|||
Get Version String |
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.
These util methods may need to have Service
in the name or similar unless robot supports some namespacing I'm unaware of @mhagen-vmware? There's also the potential for a name collision with other components in addition to the simple confusion when reading their use inline.
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.
The original intent was to put this resource file in the group directory, as that was the closest pattern I could find to namespacing.
I was asked to move the file to the resources directory, so I put the group name in the file name instead.
Generally, I'd strongly prefer to find a pattern to allow scoping of keywords over attempting to name each keyword verbosely, for all of the reasons we would avoid defining everything at the global scope in the product code.
Codecov Report
@@ Coverage Diff @@
## master #7290 +/- ##
=======================================
Coverage 26.34% 26.34%
=======================================
Files 37 37
Lines 5189 5189
=======================================
Hits 1367 1367
Misses 3715 3715
Partials 107 107 Continue to review full report at Codecov.
|
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.
Looks good!
*** Keywords *** | ||
Setup | ||
Start VIC Machine Server | ||
Install VIC Appliance To Test Server certs=${true} additional-args=--ops-user=%{TEST_USERNAME} --ops-password=%{TEST_PASSWORD} --debug 1 |
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.
We may need to tag this keyword as secret (or move this one call into a separate "Secret Setup" keyword tagged secret) because we're using %{TEST_PASSWORD}
(and don't want that logged).
*** Keywords *** | ||
Secret Install VIC Appliance With Ops Credentials | ||
[Tags] secret | ||
Install VIC Appliance To Test Server certs=${true} additional-args=--ops-user=%{TEST_USERNAME} --ops-password=%{TEST_PASSWORD} --debug 1 |
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.
this is just setting the ops user to administrator@vsphere.local... not sure that really is what we want to do, also wrapping this entire thing in [tags] secret is going to be a real problem when it comes to debugging.
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.
this is just setting the ops user to administrator@vsphere.local... not sure that really is what we want to do
in order to not duplicate with CLI's tests, here it only checks that the ops user configuration goes into VCH config (inspect output matches up with what we put in install parameter). So it doesn't really matter what ops user we set it to.
wrapping this entire thing in [tags] secret is going to be a real problem when it comes to debugging
Since we're setting ops user, I guess we don't want to expose %{TEST_USERNAME}
and %{TEST_PASSWORD}
in the log?
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.
@jzt is adding an ops user to our CI already, just use that and remove the tags secret from here.
|
||
Verify VCH List Empty | ||
|
||
[Teardown] NONE |
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 just removed this test here:
https://github.com/vmware/vic/pull/7879/files
Please do not add it back in. It inherently cannot be run in our CI, it needs to be moved to manual tests
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.
oh sorry I didn't see it. It got automatically added by git merge. will remove it now.
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.
just removed
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.
Lgtm
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.
lgtm
(cherry picked from commit 21f713d)
[specific ci=23-04-VCH-Inspect --suite 23-02-VCH-List]
Fixes #6031
some basic robot tests added for API inspect.