Skip to content

e2e: eg install and uninstall test#3515

Closed
ShyunnY wants to merge 1 commit intoenvoyproxy:mainfrom
ShyunnY:install-uninstall-e2e
Closed

e2e: eg install and uninstall test#3515
ShyunnY wants to merge 1 commit intoenvoyproxy:mainfrom
ShyunnY:install-uninstall-e2e

Conversation

@ShyunnY
Copy link
Copy Markdown
Contributor

@ShyunnY ShyunnY commented Jun 2, 2024

What type of PR is this?
e2e: eg install and uninstall test

What this PR does / why we need it:

Which issue(s) this PR fixes:
This PR adds e2e tests for the parts of Helm.PackageTool used in egctl.

  1. I set both install/uninstall to the same ConformanceTest. This avoids duplicating the install/uninstall process for separate tests
  2. Minor refactoring of the upgrade part, which I think belongs to the package manager feature.

Fixes #3323

@ShyunnY ShyunnY requested a review from a team as a code owner June 2, 2024 10:55
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.44%. Comparing base (a2e9bb5) to head (aefac57).
Report is 1109 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3515      +/-   ##
==========================================
+ Coverage   67.36%   67.44%   +0.08%     
==========================================
  Files         182      182              
  Lines       22433    22433              
==========================================
+ Hits        15111    15130      +19     
+ Misses       6230     6216      -14     
+ Partials     1092     1087       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jun 2, 2024

/retest

shawnh2
shawnh2 reviewed Jun 3, 2024
View reviewed changes
Comment thread test/e2e/tests/eg_uninstall_install.go Outdated
Comment thread test/e2e/tests/tests.go Outdated
@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jun 5, 2024

/retest

arkodg
arkodg reviewed Jun 14, 2024
View reviewed changes
Comment thread test/config/gatewayclass.yaml Outdated
@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jun 20, 2024

/retest

@guydc
Copy link
Copy Markdown
Contributor

guydc commented Jun 25, 2024 •
edited
Loading

maybe use the term lifecycle instead of package/upgrade? It's slightly broader.

@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jun 26, 2024

maybe use the term lifecycle instead of package/upgrade? It's slightly broader.

sounds good!

@guydc
Copy link
Copy Markdown
Contributor

guydc commented Jun 26, 2024

Hi @ShyunnY ! should we also update the existing "upgrade" test to use the new helm util?

@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jun 26, 2024 •
edited
Loading

Hi @ShyunnY ! should we also update the existing "upgrade" test to use the new helm util?

@guydc
I'm not sure if what you mean by "update the existing "upgrade" test to use the new helm util" is: we should modify the logic in e2e/upgrade.go to use the tool in internal/utils/helm/package.go for the upgrade.
Hopefully my understanding is correct :)

@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jun 28, 2024

@guydc

I thought about it, let's solve the current PR first. In the future I will open an issue: about adding egctl upgrade feature, I will enhance helm util and let upgrade e2e use helm util.
What do you think?

@guydc
Copy link
Copy Markdown
Contributor

guydc commented Jun 28, 2024

yes, sure, let's get this in first.

@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jul 1, 2024

/retest

arkodg
arkodg reviewed Jul 1, 2024
View reviewed changes
Comment thread test/config/gatewayclass.yaml Outdated
@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jul 4, 2024

hi, @arkodg
I'm sorry for the delay in finishing this PR, I've done!

@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jul 4, 2024

/retest

@ShyunnY ShyunnY mentioned this pull request Jul 4, 2024
Open
guydc
guydc reviewed Jul 4, 2024
View reviewed changes
Comment thread test/e2e/tests/eg_lifecycle.go Outdated
@guydc
Copy link
Copy Markdown
Contributor

guydc commented Jul 5, 2024

In addition, we specify the order of execution in the Makefile, will this still affect other test suites?

It will not impact other suites, but may impact other tests within this suite.

@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jul 5, 2024

In addition, we specify the order of execution in the Makefile, will this still affect other test suites?

It will not impact other suites, but may impact other tests within this suite.

can you give an example? Thanks!
Or can we specify the order in which the suites are executed?

@guydc
Copy link
Copy Markdown
Contributor

guydc commented Jul 5, 2024 •
edited
Loading

In addition, we specify the order of execution in the Makefile, will this still affect other test suites?

It will not impact other suites, but may impact other tests within this suite.

can you give an example? Thanks! Or can we specify the order in which the suites are executed?

One example that comes to mind:

  • Shutdown Test: if Uninstall/Install test executes before shutdown test, shutdown test will run on v0.0.0-latest instead of main. A regression in the shutdown manager component may go undiscovered in the PR, as main is not deployed during that test.

I think that we can just seperate this test into another suite and ensure that it runs after everything else in the run-e2e make target, or explicitly control the order of tests in the upgrade/lifecycle suite.

@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jul 6, 2024

@guydc

wow, I like this solution ^_^

IMO, we should try not to introduce unnecessary complexity in E2E testing, which may cause additional confusion for new users. I will take your solution and change it, thank you for your review.

@ShyunnY ShyunnY requested review from arkodg and guydc July 7, 2024 14:44
@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jul 7, 2024

/retest

guydc
guydc reviewed Jul 10, 2024
View reviewed changes
Comment thread test/e2e/lifecycle/eg_lifecycle_test.go Outdated
@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jul 14, 2024

It seems like we've been procrastinating for a long time, can we please get on with this work? :)

shawnh2
shawnh2 previously approved these changes Jul 14, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

LGTM! Probably need to wait #3764 gets in first.

@arkodg arkodg requested a review from guydc July 15, 2024 18:33
@ShyunnY ShyunnY force-pushed the install-uninstall-e2e branch from 769193f to 70eb4dd Compare July 21, 2024 14:29
@ShyunnY
e2e: add lifecycle test
aefac57 
Signed-off-by: shyunny <shyunny@outlook.com>
@ShyunnY ShyunnY force-pushed the install-uninstall-e2e branch from 70eb4dd to aefac57 Compare July 21, 2024 15:03
@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jul 21, 2024

I'm sorry for delaying the resolution of this conflict.

In the latest commit, I thought to simplify the lifecycle process as much as possible. So I executed it as a separate E2E Test and scheduled it to be executed in the last task. Avoid affecting the rest of the E2E.

@ShyunnY
Copy link
Copy Markdown
Contributor Author

ShyunnY commented Jul 22, 2024

/retest

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 21, 2024

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale label Aug 21, 2024
@github-actions github-actions Bot removed the stale label May 20, 2025
@arkodg
Copy link
Copy Markdown
Contributor

arkodg commented May 23, 2025

closing this PR since its become inactive, feel free to reopen if you're still working on it

@arkodg arkodg closed this May 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shawnh2 shawnh2 shawnh2 left review comments

@arkodg arkodg Awaiting requested review from arkodg

@guydc guydc Awaiting requested review from guydc

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

e2e: add egctl install/uninstall test

4 participants

@ShyunnY @guydc @arkodg @shawnh2