Skip to content
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

Improve docs for running local integration tests #8145

Merged
merged 2 commits into from
Aug 29, 2018
Merged

Conversation

zjs
Copy link
Member

@zjs zjs commented Jul 17, 2018

The instructions for using local-integration-test.sh were incomplete. Improve them to reflect the current requirements for using the script.

The instructions for using local-integration-test.sh were incomplete.
Improve them to reflect the current requirements for using the script.
@zjs zjs requested a review from a team as a code owner July 17, 2018 05:54
@zjs zjs requested review from DanielXiao and hickeng July 17, 2018 05:55
@zjs zjs self-assigned this Jul 17, 2018
tests/README.md Outdated

This script requires that `govc` is [installed][govc-install] and available on
your `PATH`; that `docker` is installed, available on your `PATH`, and usable by
your current user; and that the following environment variables are defined:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script also requires drone.

Copy link
Member

@hickeng hickeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noted areas of uncertainty for follow up, but even without addressing these comments this is an improvement on the current docs.

@@ -1,10 +1,29 @@
# VIC Engine Integration & Functional Test Suite

To run the integration tests locally:
Integration tests can be run locally in several different ways.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to need a glossary of terms that defines what we mean by unit/functional/integration/scenario/etc. Not for this PR, but reading the usage reminded me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A glossary would be helpful, but we now introduce these terms in CONTRIBUTING.md.

tests/README.md Outdated

## Automatic with defaults

Use ./local-integration-test.sh
The recommended way to run integration tests is [local-integration-test.sh][sh].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recommended way to run integration tests locally is ....

tests/README.md Outdated
Use ./local-integration-test.sh
The recommended way to run integration tests is [local-integration-test.sh][sh].

This script requires that `govc` is [installed][govc-install] and available on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script file currently requires a minimum version of 0.9.0 and will provide a sane error if the version isn't met.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In part because the script provides a sane error, I chose not to document that requirement here. (This is already wordy enough, and I hope that someone following these instructions will download the latest version.)

tests/README.md Outdated
The recommended way to run integration tests is [local-integration-test.sh][sh].

This script requires that `govc` is [installed][govc-install] and available on
your `PATH`; that `docker` is installed, available on your `PATH`, and usable by
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also a minimum version of docker client required but I'm not actually sure what that was. I think it was 1.13.0 (a customer has had issues with cert validation via IP with 1.13.1) but should confirm.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this version requirement imposed by the product, the tests, or the script?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

product.

in the target environment.
* `PUBLIC_NETWORK` (optional): The public network tests should use, if multiple
exist in the target environment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are some assumptions about bridge network as well, and use of specific VLAN ranges that were added when shifting to VC as a primary environment. I'm not sure that these have been fully parameterized out or written to function in the absence of VC or VLAN support.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, I don't do any explicit configuration of the bridge network when configuring my VC or running these tests. It's possible this happens automatically as a result of the way my VC is created— if I figure out what configuration is required, I'll update these instructions.

3. If there is an existing VCH available, it is possible to bypass the VCH installation/deletion by adding a TARGET_VCH into the list of test secrets.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is only available for Group1 at this time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. I don't think I've actually changed this section though (other than my editor adding the missing newline at the end of the file) and there are enough other things to update here that it's probably worth creating a separate PR.

Copy link
Member

@hickeng hickeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noted areas of uncertainty for follow up, but even without addressing these comments this is an improvement on the current docs.

@zjs zjs merged commit 4685457 into vmware:master Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants