Skip to content

Better integration tests#2406

Merged
dgageot merged 6 commits intoGoogleContainerTools:masterfrom
dgageot:better-integration-tests
Jul 5, 2019
Merged

Better integration tests#2406
dgageot merged 6 commits intoGoogleContainerTools:masterfrom
dgageot:better-integration-tests

Conversation

@dgageot
Copy link
Copy Markdown
Contributor

@dgageot dgageot commented Jul 4, 2019

  • Switch to latest TravisCI env (xenial) because it seems faster and has more disk space for integration tests
  • Print more information when an integration test goes wrong
  • Re-enable TestDev that wasn't flaky
  • Increase the duration of the test suite to increase the chance of running all the tests when one goes wrong.

dgageot added 5 commits July 4, 2019 12:02
@dgageot
Same indentation everywhere
a8fd6f2 
Signed-off-by: David Gageot <david@gageot.net>
@dgageot
Switch to most recent TravisCI env
4dcb1b6 
Signed-off-by: David Gageot <david@gageot.net>
@dgageot
Print k8s nodes and disk space in case of error
c6e79e5 
Signed-off-by: David Gageot <david@gageot.net>
@dgageot
Print k8s nodes and disk space before running tests
426b5d4 
Signed-off-by: David Gageot <david@gageot.net>
@dgageot
Increase test suite duration
dd4bb1d 
Signed-off-by: David Gageot <david@gageot.net>
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 4, 2019 •
edited
Loading

Codecov Report

Merging #2406 into master will not change coverage.
The diff coverage is n/a.

@dgageot dgageot force-pushed the better-integration-tests branch from d6761d6 to b056811 Compare July 4, 2019 15:11
@dgageot
Copy link
Copy Markdown
Contributor Author

dgageot commented Jul 4, 2019 •
edited
Loading

Green integration tests: 7 times
There's still a flaky unit test that I'm investigating

@dgageot dgageot force-pushed the better-integration-tests branch 5 times, most recently from 70d995a to eaf3532 Compare July 5, 2019 07:59
@dgageot
Restore TestDev test
57fb190 
Signed-off-by: David Gageot <david@gageot.net>
@dgageot dgageot force-pushed the better-integration-tests branch from eaf3532 to 57fb190 Compare July 5, 2019 08:33
@dgageot
Copy link
Copy Markdown
Contributor Author

dgageot commented Jul 5, 2019

@corneliusweig would you mind taking a look at this PR? I'm trying to fix all known flaky tests

@corneliusweig
Copy link
Copy Markdown
Contributor

corneliusweig commented Jul 5, 2019

Sure, I can do that.

corneliusweig
corneliusweig approved these changes Jul 5, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Looks good. I just have two questions.

Makefile
endif
GCP_ONLY=$(GCP_ONLY) go test -v $(REPOPATH)/integration -timeout 15m $(INTEGRATION_TEST_ARGS)
kubectl get nodes -oyaml
GCP_ONLY=$(GCP_ONLY) go test -v $(REPOPATH)/integration -timeout 20m $(INTEGRATION_TEST_ARGS)
Copy link
Copy Markdown
Contributor

@corneliusweig corneliusweig Jul 5, 2019

Choose a reason for hiding this comment

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

I don't fully understand how this relates to your comment:

Increase the duration of the test suite to increase the chance of running all the tests when one goes wrong.

Do you mean that this gives enough time for the remaining tests to finish, so that they are not marked as failed?

Copy link
Copy Markdown
Contributor Author

@dgageot dgageot Jul 5, 2019

Choose a reason for hiding this comment

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

Exactly! It's so better said!

integration/util.go
select {
case <-ctx.Done():
k.printDiskFreeSpace()
k.debug("nodes")
Copy link
Copy Markdown
Contributor

@corneliusweig corneliusweig Jul 5, 2019

Choose a reason for hiding this comment

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

Does k.debug("nodes") add anything? After all, the nodes are already unconditionally printed in make integration (https://github.com/GoogleContainerTools/skaffold/pull/2406/files#diff-b67911656ef5d18c4ae36cb6741b7965R112).

Copy link
Copy Markdown
Contributor Author

@dgageot dgageot Jul 5, 2019

Choose a reason for hiding this comment

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

When the TestDebug used to fail, I could see an issue with the k8s nodes.
I added those logs before the test suite and in case of an error, to detect cases where the kind cluster might get broken after running a few tests.

Copy link
Copy Markdown
Contributor

@corneliusweig corneliusweig Jul 5, 2019

Choose a reason for hiding this comment

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

Ok, that makes sense.

@dgageot dgageot merged commit b9e355d into GoogleContainerTools:master Jul 5, 2019
@dgageot dgageot mentioned this pull request Jul 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@balopat balopat Awaiting requested review from balopat

@nkubala nkubala Awaiting requested review from nkubala

@priyawadhwa priyawadhwa Awaiting requested review from priyawadhwa

@tejal29 tejal29 Awaiting requested review from tejal29

1 more reviewer

@corneliusweig corneliusweig corneliusweig approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@corneliusweig corneliusweig

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dgageot @corneliusweig @googlebot