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.

Sign up for GitHub

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

Jump to bottom

Cloud: Make e2e Tests Less Chatty, and More Stable #29883

Merged
merged 3 commits into from
Nov 15, 2021
Merged

Cloud: Make e2e Tests Less Chatty, and More Stable #29883

merged 3 commits into from
Nov 15, 2021

Conversation

barrettclark
Copy link
Contributor

@barrettclark barrettclark commented Nov 3, 2021 •
edited by omarismail
Loading

Description

This fixes the Cloud e2e tests.

Changes

  • Run table tests as subtests (in parallel)
  • Fix broken tests
  • Remove TF_LOG=INFO in test cases
  • Add a readme to show you to run tests.
  • Add new -tfoutput flag to print the terraform to standard output.
  • Change expect to not log to STDOUT
  • Some commands were changed to use -auto-approve because it wasn't necessary for extra input steps.

Note: I've removed some t.Parallel() bits because it started introducing some flakiness. I didn't do a full investigation because of time considerations. We will investigate this at a later point.

Test Output

terraform % TF_ACC=1 go test ./internal/cloud/e2e/... -v -ldflags "-X \"github.com/hashicorp/terraform/version.Prerelease=alpha20211029\"" 
...
...
PASS
ok      github.com/hashicorp/terraform/internal/cloud/e2e       489.176s

@barrettclark barrettclark added cloud Related to Terraform Cloud's integration with Terraform 1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Nov 3, 2021
@barrettclark barrettclark changed the title Fix broken cloud e2e tests Cloud: Make e2e Tests Less Chatty, and More Stable Nov 4, 2021
@barrettclark barrettclark force-pushed the barrettclark/cloud-e2e-tests branch from 1010ea5 to 0155893 Compare November 4, 2021 20:47
@hashicorp hashicorp deleted a comment from barrettclark Nov 8, 2021
@omarismail omarismail force-pushed the barrettclark/cloud-e2e-tests branch 4 times, most recently from cb46ba1 to 0ec40de Compare November 11, 2021 15:18
@omarismail omarismail marked this pull request as ready for review November 11, 2021 15:30
apparentlymart
apparentlymart approved these changes Nov 11, 2021
View reviewed changes
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

I'm far from expert on the specific behaviors these tests are exercising and so I focused my review on the general ideas (use subtests, parallel, etc) rather than the specific test case, and it all makes sense to me. I'll leave final review to folks who are more familiar with the code being tested here, in case there's something specific about this integration that I'm not aware of.

I did leave a question inline about the possibly-redundant(?) mechanisms for enabling these tests, but as I said inline I'm okay with it if there's something unique here which justifies it; just wanted to point out the inconsistency with other e2etest-ish packages in case it was an accidental one.

go.mod
github.com/hashicorp/go-version v1.2.1
github.com/hashicorp/go-version v1.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

For the benefit of other potential reviewers: this bump seems to just introduce the version.Core method, from hashicorp/go-version#85, and so shouldn't lead to any behavior changes for other parts of Terraform not using that new method.

internal/cloud/e2e/README.md Outdated Show resolved Hide resolved
@omarismail omarismail force-pushed the barrettclark/cloud-e2e-tests branch from 0ec40de to 0cb9310 Compare November 11, 2021 18:45
nfagerlund
nfagerlund reviewed Nov 11, 2021
View reviewed changes
Copy link
Member

@nfagerlund nfagerlund left a comment

Choose a reason for hiding this comment

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

I'm still trying to get a successful run of all e2e tests, and I'd like to try out that -tfoutput flag, but in the meantime here are my code comments!

internal/cloud/e2e/README.md Outdated Show resolved Hide resolved
internal/cloud/e2e/apply_auto_approve_test.go Outdated Show resolved Hide resolved
internal/cloud/e2e/README.md Show resolved Hide resolved
internal/cloud/e2e/helper_test.go Outdated Show resolved Hide resolved
internal/cloud/e2e/helper_test.go Outdated Show resolved Hide resolved
internal/cloud/e2e/helper_test.go Outdated Show resolved Hide resolved
@omarismail omarismail force-pushed the barrettclark/cloud-e2e-tests branch 2 times, most recently from 00c0ab9 to 7758216 Compare November 11, 2021 19:36
@barrettclark @omarismail
Cloud: fix e2e tests
5ef82dd 
- Fix tests and remove commented code
- Remove parallel for some flaky tests
- Add README
@omarismail omarismail force-pushed the barrettclark/cloud-e2e-tests branch from 7758216 to 5ef82dd Compare November 11, 2021 19:37
@omarismail omarismail force-pushed the barrettclark/cloud-e2e-tests branch from 63d780b to 7af4cff Compare November 11, 2021 20:27
@omarismail omarismail force-pushed the barrettclark/cloud-e2e-tests branch from 7af4cff to 57a4b51 Compare November 11, 2021 21:33
@apparentlymart apparentlymart added this to the v1.1.0 milestone Nov 13, 2021
@omarismail omarismail force-pushed the barrettclark/cloud-e2e-tests branch from 4fac1bd to 7211cb7 Compare November 14, 2021 21:32
@omarismail omarismail force-pushed the barrettclark/cloud-e2e-tests branch from 7211cb7 to 9b675f8 Compare November 15, 2021 15:36
@barrettclark
Copy link
Contributor Author

The tests pass for me running against tfcdev locally and also Oasis. I had to increase the timeout for the local run, which took about 12 minutes to run. The default test timeout is 10 minutes. Oasis only took 8 minutes.

omarismail
omarismail approved these changes Nov 15, 2021
View reviewed changes
Copy link
Contributor

@omarismail omarismail left a comment

Choose a reason for hiding this comment

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

tests pass for me. 🚢 ✅

@barrettclark barrettclark merged commit 388c430 into main Nov 15, 2021
@barrettclark barrettclark deleted the barrettclark/cloud-e2e-tests branch November 15, 2021 16:45
@teamterraform teamterraform mentioned this pull request Nov 15, 2021
Merged
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nfagerlund nfagerlund nfagerlund left review comments

@apparentlymart apparentlymart apparentlymart approved these changes

@omarismail omarismail omarismail approved these changes

@chrisarcand chrisarcand Awaiting requested review from chrisarcand

@alisdair alisdair Awaiting requested review from alisdair

Assignees
No one assigned
Labels
1.1-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged cloud Related to Terraform Cloud's integration with Terraform
Projects
None yet
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.
4 participants
@barrettclark @apparentlymart @nfagerlund @omarismail