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

Automated test plan #338

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Automated test plan #338

wants to merge 4 commits into from

Conversation

edipascale
Copy link
Contributor

No description provided.

@edipascale edipascale force-pushed the ema/peering-test branch 2 times, most recently from dd013f7 to 6b55803 Compare January 23, 2025 14:29
@Frostman
Copy link
Member

@edipascale could you please make a separate PR with the things refactoring in the testing.go so we can get it merged quickly and you'll not have to rebase

@edipascale edipascale force-pushed the ema/peering-test branch 5 times, most recently from beb5602 to 0cc35cd Compare January 29, 2025 11:53
Copy link

github-actions bot commented Jan 29, 2025

Test Results

16 tests   11 ✅  1h 31m 40s ⏱️
 3 suites   0 💤
 1 files     5 ❌

For more details on these failures, see this check.

Results for commit d6f53c0.

♻️ This comment has been updated with latest results.

@edipascale edipascale force-pushed the ema/peering-test branch 2 times, most recently from 36a3864 to 08c5385 Compare January 30, 2025 08:13
@edipascale edipascale added the release-test Run release test CI job label Jan 30, 2025
@edipascale edipascale force-pushed the ema/peering-test branch 11 times, most recently from 054c3d9 to 8283482 Compare February 4, 2025 18:05
@edipascale edipascale marked this pull request as ready for review February 6, 2025 17:20
@edipascale edipascale requested a review from Frostman as a code owner February 6, 2025 17:20
@edipascale edipascale force-pushed the ema/peering-test branch 2 times, most recently from 89eb0e7 to fbc774c Compare February 11, 2025 09:26
Copy link
Member

@Frostman Frostman left a comment

Choose a reason for hiding this comment

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

Great job! I wasn't digging much into each test case, mainly looking for potential issues, unclear behavior, and general Go code issues.

Few notes for fixing in future PRs:

  • I have a few questions about making sure that tests that are suitable for the VLAB will run
  • failed cleanups in defer may leave fabric in a semi-broken state
  • make SKIPed (b/c some requirements are missing) tests not FAIL (let's discuss)
  • overall report in the could be a bit more clear (prev item probably contributes a lot to it)

@edipascale edipascale force-pushed the ema/peering-test branch 3 times, most recently from 80668fb to e2d0f80 Compare March 5, 2025 15:12
@edipascale edipascale marked this pull request as draft March 5, 2025 20:10
@edipascale edipascale force-pushed the ema/peering-test branch 4 times, most recently from d4a4e9e to e726a86 Compare March 7, 2025 09:28
Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
test can carry flags which specify conditions under which they
should be skipped, e.g. if they require externals or if they are
incompatible with virtual switches.

Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
Signed-off-by: Emanuele Di Pascale <emanuele@githedgehog.com>
@edipascale
Copy link
Contributor Author

@Frostman I think I've addressed all of your comments. A couple of standing issues:

  • a run now takes ~2h5m in env-1. I tried cutting corners but short of disabling iperfs (which we agreed we cannot afford to do) the only other option I can think of is to skip entire tests. There is an extended flag which is set to false by default, but currently it only disables curls and some extra steps in a few tests. Infra is already there to skip entire tests based on that flag, if desired.
  • vlab release-tests in the ci have been enabled using the same label trick, but based on many runs on my local machine I had to disable several tests, i.e. all failover ones, the static external and all of those with "restrictions". Then there are those that won't run because of the lack of externals, and the final result is that there's only a couple of tests left.
  • even on real envs, tests seem to be quite brittle. I've had a few runs with no issues, but many more with one/two random failures, typically on a ping check, pretty much on any of the tests with no discernible pattern. I've also seen one failure with a slightly below target iperf speed (8490 vs 8500). Unless I missed something, we are already waiting for switches to be ready with an "applied for" timer of at least 30 seconds in all instances where I've observed failures, but it doesn't seem to be enough. I could try to increase the timer further, but that would make the runs even longer, and regardless I'm not sure where we should set the acceptance bar or how long is reasonable to wait for

Feel free to have a look and/or let me know if you want to discuss this over a call!

@Frostman Frostman marked this pull request as ready for review March 7, 2025 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-test Run release test CI job
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants