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

Access log tests are not useful when running tests concurrently #1084

Open
howardjohn opened this issue May 22, 2024 · 4 comments
Open

Access log tests are not useful when running tests concurrently #1084

howardjohn opened this issue May 22, 2024 · 4 comments

Comments

@howardjohn
Copy link
Member

telemetry::testing::assert_contains(want);

rust runs tests concurrently by default. We may get false-positives by matching other tests logs instead of our tests.

--test-threads 1 or cargo-nextest can weed these out, but it would be nice to somehow have per-test log collection. Note sure how though.

FWIW I always use cargo-nextest so as long as I am running tests locally we will find these issues quickly. However, that is a terrible long term stance 🙂

howardjohn added a commit to howardjohn/ztunnel that referenced this issue May 22, 2024
We were using the `cfg` in pool since istio#1040. `cfg` is not whether its
used, but rather the intent of the user -- we may enable or disable at
runtime.

This renames the field to make it clear it should not be used like this,
and fixes the issue. This went through CI due to
istio#1084.
@bleggett
Copy link
Contributor

Can we just use nextest in CI? I don't see why not.

@howardjohn
Copy link
Member Author

We could, although that also leaves the inverse possible broken test (only works when not run concurrently)... either way we lose. Using nextest is good for other reasons though, probably a decent choice. We will need to add to build tools though

@bleggett
Copy link
Contributor

We could, although that also leaves the inverse possible broken test (only works when not run concurrently)... either way we lose.

Fine with that tradeoff, as those sorts of tests would almost assuredly show up as flakes in concurrent runs anyway.

istio-testing pushed a commit that referenced this issue May 28, 2024
* Fix regression in source IP spoofing

We were using the `cfg` in pool since #1040. `cfg` is not whether its
used, but rather the intent of the user -- we may enable or disable at
runtime.

This renames the field to make it clear it should not be used like this,
and fixes the issue. This went through CI due to
#1084.

* cleanup
@bleggett
Copy link
Contributor

bleggett commented Jun 3, 2024

We should be able to use cargo test -- --shuffle eventually to flag flakes faster without nextest - currently that's behind unstable/nightly tho, better to wait for that.

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

No branches or pull requests

2 participants