-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Create a BDD Test Automation Framework with ginkgo #552
Create a BDD Test Automation Framework with ginkgo #552
Conversation
Do we need this? For the libvirt tests, I was hoping to run the usual origin e2e suite. And if you're only concerned about exercising |
Hit @wking Now, in the test workflow, the idea is to run the config tests before the cluster health tests because some issues can be found without the cluster deploy saving some time. |
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.
Overall, this looks good to me. Just a few minor requests.
|
Hi @abhinavdahiya, |
This looks like a good first step, Thanks a lot @alejovicu :yay: /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, alejovicu 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 |
/refresh |
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 left a few nits inline. Nothing that needs to block a merge, but probably things that are worth fixing post-merge.
Specify("Then I expect to see the install-config.yaml file in the path ./scenario1/", func() { | ||
Ω(openshiftInstallerPath + "/scenario1/install-config.yaml").Should(BeAnExistingFile()) | ||
}) | ||
Specify("And I expect install-config.yaml file contains the info specified in the environment variables", func() { |
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: these aren't environment variables any more (and also in several other Specify
strings in this commit).
cmd.Stderr = &errb | ||
err := cmd.Run() | ||
if err != nil { | ||
fmt.Println("Error: failed running the command \"" + command + "\" with the arguments:") |
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 could be more robust and compact with:
fmt.Printf("Error: failed running the command %q with the arguments:", command)
although printing straight to the terminal feels strange anyway. Does that get captured somewhere? Should we be logging these instead, or attaching it to the error before passing it to log.Fatal
?
var installState installStateConf | ||
json.Unmarshal(byteValue, &installState) | ||
|
||
AssertStringContains(installState.InstallConfig.Config.BaseDomain, |
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 and later parts of ValidateInstallStateConfig
feel like a hand-crafted special case of reflect.DeepEqual
. Is it doing something else?
expectedData.ClusterName, | ||
"*installconfig.clusterName.ClusterName not found in "+fileName) | ||
|
||
// OPENSHIFT_INSTALL_PLATFORM is implicitly validated by next checks. If its not Libvirt platform, the Name and URI will not be accessible |
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.
/retest Please review the full test history for this PR and help us cut down flakes. |
@alejovicu: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
From Trevor: For regions like eu-west-3 (Paris) that don't support m4. The only instance type available in all AWS regions today is i3. We prefer m4 because of the higher EBS attach limit. But for regions that don't support m4 we use m5. This also adds a CI job which checks the AWS pricing API to keep our region map up to date. The eu-north-1 region is new as of 2018-12-12 [1], and it show up in: $ aws ec2 describe-regions --query 'Regions[].RegionName' --output json | jq -r 'sort[]' ... eu-central-1 eu-north-1 eu-west-1 ... The GovCloud entries didn't show up in that query (maybe they are only listed for accounts that have access?), but they are listed on [2]. The new test is in platformtests/aws to keep it out of our default unit tests. Maintainers can run it manually to verify that the default fallbacks we hard-code are still appropriate for the current AWS offerings. It's not in tests/aws, because 'dep ensure' seems to ignore tests. I think it's fine to have both platformtests and tests in parallel, with the former holding tests who share the project vendor/ and the latter holding tests which have a distinct vendor/. bdd-smoke is in tests with its own vendor/ intentionally to keep gomega and other test-specific libraries out of the project vendor/ [3]. But for these tests we're using Go's built-in test framework, so it's less maintenance to layer our minimal additional dependencies into the project vendor/. [1]: https://aws.amazon.com/blogs/aws/now-open-aws-europe-stockholm-region/ [2]: https://aws.amazon.com/about-aws/global-infrastructure/regional-product-services/ [3]: openshift#552 (comment)
From Trevor: For regions like eu-west-3 (Paris) that don't support m4. The only instance type available in all AWS regions today is i3. We prefer m4 because of the higher EBS attach limit. But for regions that don't support m4 we use m5. This also adds a CI job which checks the AWS pricing API to keep our region map up to date. The eu-north-1 region is new as of 2018-12-12 [1], and it show up in: $ aws ec2 describe-regions --query 'Regions[].RegionName' --output json | jq -r 'sort[]' ... eu-central-1 eu-north-1 eu-west-1 ... The GovCloud entries didn't show up in that query (maybe they are only listed for accounts that have access?), but they are listed on [2]. The new test is in platformtests/aws to keep it out of our default unit tests. Maintainers can run it manually to verify that the default fallbacks we hard-code are still appropriate for the current AWS offerings. It's not in tests/aws, because 'dep ensure' seems to ignore tests. I think it's fine to have both platformtests and tests in parallel, with the former holding tests who share the project vendor/ and the latter holding tests which have a distinct vendor/. bdd-smoke is in tests with its own vendor/ intentionally to keep gomega and other test-specific libraries out of the project vendor/ [3]. But for these tests we're using Go's built-in test framework, so it's less maintenance to layer our minimal additional dependencies into the project vendor/. [1]: https://aws.amazon.com/blogs/aws/now-open-aws-europe-stockholm-region/ [2]: https://aws.amazon.com/about-aws/global-infrastructure/regional-product-services/ [3]: openshift#552 (comment)
openshift-install create install-config
for libvirt platform (https://jira.coreos.com/browse/CORS-858)