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

Fix Disabled test #24

Merged
merged 4 commits into from
Apr 28, 2022
Merged

Fix Disabled test #24

merged 4 commits into from
Apr 28, 2022

Conversation

Nuru
Copy link
Contributor

@Nuru Nuru commented Apr 28, 2022

what

  • Update testing of enabled = false
  • Fix Makefile to run all tests
  • Update git.io -> cloudposse.tools

why

  • Proper test structure
  • Was running only a single test even when multiple tests were present
  • EOL for git.io

@Nuru Nuru requested review from a team as code owners April 28, 2022 18:17
@Nuru Nuru added the patch A minor, backward compatible change label Apr 28, 2022
@Nuru
Copy link
Contributor Author

Nuru commented Apr 28, 2022

/test all

mcalhoun
mcalhoun approved these changes Apr 28, 2022
View reviewed changes
Copy link
Member

@mcalhoun mcalhoun left a comment

Choose a reason for hiding this comment

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

LGTM. Just made a small comment

test/src/Makefile
@@ -15,7 +15,7 @@ init:
## Run tests
test: init
go mod download
go test -v -timeout 60m -run TestExamplesComplete
go test -v -timeout 60m
Copy link
Member

Choose a reason for hiding this comment

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

Do we want go test -v -timeout 60m ./... here to run all tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Although the tests in the example should complete in under a minute or 2, this is a template repository and by default we allow 60 minutes for tests to complete, because some things, like testing EKS auto-scaling, can take a long time to set up and tear down.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry...I was asking if we should include ./... specifically to include all tests recurively

@Nuru Nuru merged commit 2b51aed into main Apr 28, 2022
@Nuru Nuru deleted the disabled-test branch April 28, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcalhoun mcalhoun mcalhoun approved these changes

@nitrocode nitrocode Awaiting requested review from nitrocode nitrocode is a code owner automatically assigned from cloudposse/contributors

@RothAndrew RothAndrew Awaiting requested review from RothAndrew RothAndrew is a code owner automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
patch A minor, backward compatible change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Nuru @mcalhoun @cloudpossebot