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

Updates continue-on-error e2e test case #1426

Merged
merged 1 commit into from
Feb 8, 2021

Conversation

seans3
Copy link
Contributor

@seans3 seans3 commented Feb 8, 2021

  • Fixes flaky "continue-on-error" end-to-end test case. Flakiness caused by re-using namespace from previous test which is deleted before being re-created, causing a race condition. Moves test to position before deletion.
  • Updates number of inventory items from 1 to 2, since the namespace is also stored in the inventory if it applied with other items.
  • Adds a couple more assert statements.
  • Adds missing licenses to resource yamls.

Current output:

Testing continue-on-error
kpt live apply e2e/live/testdata/continue-on-error
......SUCCESS

@seans3 seans3 requested a review from runewake2 February 8, 2021 02:22
@runewake2
Copy link
Contributor

If reusing the namespace creates a race condition maybe it'd make sense to move this test into a unique namespace instead? Otherwise this may reoccur after more tests are added or things are refactored.

@mortent
Copy link
Contributor

mortent commented Feb 8, 2021

I am concerned about the maintainability of this test suite. It is currently almost 1000 lines of bash script and several folders with test data.
Should we look at turning this into Ginkgo tests, similar to how we do this in cli-utils and in OSS Kubernetes? We can just use Golang os/exec to run the kpt cli commands.

@runewake2
Copy link
Contributor

I had looked into adopting a go based exec pattern when I implimented this originally. The hope was reduced complexity and ability to get test results in testing tools/CI.

It seemed like adopting that would significantly increase the time it took to deliver these tests in the short term and I wasn't sure it made sense to tackle right now. I agree we want to move away from this pattern but without a wrapper around exec tests were a lot less easy to read then they are here.

Happy to discuss more.

@mortent
Copy link
Contributor

mortent commented Feb 8, 2021

I don't think we should hold up merging this PR, but it seems like it is time to consider how we can do e2e tests at scale.

@Liujingfang1
Copy link
Contributor

/lgtm

I think we can first enable these e2e tests in some Github jobs. When we enable it, these tests should be run against different K8s versions.
Then we consider making them more efficient and maintainable.

@seans3 seans3 merged commit 998d233 into kptdev:master Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants