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

Add Extended Tests for ANNP E2E #5486

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

qiyueyao
Copy link
Contributor

@qiyueyao qiyueyao commented Sep 14, 2023

  • Adds some extended tests for ANNP e2e as mode-irrelevant to run in test-e2e-encap.

Doc for tracking example tests, current tests, and proposed tests.

Open for discussion on running scenario:

  1. Add --extended to the current test-e2e-encap-all-features-enabled kind test on Github for each PR
  • Pros: Mort effortless change and efficient
  • Cons: Embedded into current all features test and will be automatically run for every PR

Current plan to use mode-irrelevant so the tests only run for test-e2e-encap.
2. Create a new kind test workflow on Github for each PR

  • Pros: Reasons clearly with a separate job
  • Cons: More burden to run, provided currently there is not enough extended tests, might be valuable in the future.
  1. Create a new Jenkins job triggered by /test-e2e-extended
  • Pros: Run on demand
  • Cons: new machine setup? and not clear on when this tag is required
  1. Scheduled task
  • Currently no e2e tests are run with a scheduled Github action, in this case we need to schedule the entire e2e tests + extended tests.

@Dyanngg
Copy link
Contributor

Dyanngg commented Sep 15, 2023

My take on this: I feel like a cron github action for extended set of policy e2e tests sounds like a good and extensible approach. Even though there are only a few tests that currently in the extended set introduced by this PR, in the future we could also 1. parametrize current e2e tests so that the extended tests run on different combinations of protocol/ports or even workloads 2. with a lot of discretion, move a small portion of the current test suite into extended if they appear a bit repetitive. Thoughts? @tnqn @antoninbas

@antoninbas
Copy link
Contributor

Can someone clarify why these are added as "extended" tests and not enabled by default? Do they take a long time to run or overlap significantly with other tests?

@Dyanngg
Copy link
Contributor

Dyanngg commented Sep 19, 2023

Can someone clarify why these are added as "extended" tests and not enabled by default? Do they take a long time to run or overlap significantly with other tests?

I think the motivation in general is that we want to improve the e2e test coverage while speed up test execution time for individual PR updates as much as possible. For now I don't think the the network policy e2e testcases individually takes a long time to run, but they do add up. On the other hand, for the majority of testcases that we have, only TCP traffic is validated, leaving the enforcement of other protocols vulnerable. So the 'extened set of test' idea is proposed by @salv-orlando and me, as we are looking to see if we can conclude a set of essential testcases for ACNP/ANNP, and have an extended set of tests run periodically for different variations of the same test, which does not run every single time. The testcases currently in the PR probably does not reflect it well @qiyueyao, but this was the overarching idea

@antoninbas
Copy link
Contributor

  • Instead of defining a new flag (--extended), I think we should re-use what we already have (the --skip flag). Look for mode-irrelevant in the code base.
  • Would a good starting point be to run the new "extended" tests for a single Kind configuration (e.g., encap mode only)? This way the tests are still run for every PR, but only once. We can revisit in the future if we have more tests like this.

In general, I recommend only adding an e2e test to the "extended" set if it takes a while to run or if it provides little benefit. Tests which are not run for every PR can easily become broken or start failing. When running a test suite as a periodical job, it is pretty common for failures to go unnoticed for a while, with no one checking up on it or investigating.

@@ -131,6 +131,10 @@ case $key in
skiplist="$2"
shift 2
;;
--extended)
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to keep the changes in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops not intended, and will continue to add some tests, not finished yet, thanks!

Copy link
Contributor

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 11, 2024
Add extended e2e test cases for ANNP run in encap mode only.

Signed-off-by: Qiyue Yao <[email protected]>
@github-actions github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 10, 2024
Copy link
Contributor

github-actions bot commented Jul 9, 2024

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 9, 2024
@tnqn tnqn removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 9, 2024
@qiyueyao
Copy link
Contributor Author

qiyueyao commented Jul 9, 2024

Wondering if we still have a plan to add extended E2E tests for NetworkPolicy? Since we are actually reducing test burden, otherwise I will update this PR. @tnqn @Dyanngg

@tnqn
Copy link
Member

tnqn commented Jul 10, 2024

Wondering if we still have a plan to add extended E2E tests for NetworkPolicy? Since we are actually reducing test burden, otherwise I will update this PR. @tnqn @Dyanngg

I think this wouldn't add much test burden if the extended tests run only in one kind job and if you will move some repetitive existing tests to it. So adding this still looks good to me. I just missed the PR. If you decide to update it, I think I can prioritize it after 2.1 is released.

Copy link
Contributor

github-actions bot commented Oct 9, 2024

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 9, 2024
@tnqn tnqn removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 9, 2024
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.

4 participants