fix(tests): Make tests pass with `cargo test` #23520

thomasqueirozb · 2025-08-04T17:45:57Z

Summary

cargo test would hang/fail on master both on MacOS and Linux.

Vector configuration

NA

How did you test this PR?

cargo test

Change Type

Bug fix
New feature
Non-functional (chore, refactoring, docs)
Performance

Is this a breaking change?

Yes
No

Does this PR include user facing changes?

Yes. Please add a changelog fragment based on our guidelines.
No. A maintainer will apply the no-changelog label to this PR.

References

Closes: cargo test sometimes fails to run on master #23189

Notes

Please read our Vector contributor resources.
Do not hesitate to use @vectordotdev/vector to reach out to us regarding this PR.
Some CI checks run only after we manually approve them.
- We recommend adding a pre-push hook, please see this template.
- Alternatively, we recommend running the following locally before pushing to the remote branch:
  - cargo fmt --all
  - cargo clippy --workspace --all-targets -- -D warnings
  - cargo nextest run --workspace (alternatively, you can run cargo test --all)
After a review is requested, please avoid force pushes to help us review incrementally.
- Feel free to push as many commits as you want. They will be squashed into one before merging.
- For example, you can run git merge origin master and git push.
If this PR introduces changes Vector dependencies (modifies Cargo.lock), please
run cargo vdev build licenses to regenerate the license inventory and commit the changes (if any). More details here.

src/sources/socket/mod.rs

thomasqueirozb · 2025-08-04T17:50:12Z

@jorgehermo9 this touches some tests you added in case you want to take a look

pront · 2025-08-04T17:50:14Z

src/sources/exec/tests.rs

@@ -329,6 +329,7 @@ async fn test_drop_receiver() {

 #[tokio::test]
 #[cfg(unix)]
+#[cfg_attr(target_os = "macos", ignore)] // Flaky when running `cargo test`


do we know the reason?

Unfortunately not, tried to debug briefly but didn't get anywhere

Ok no worries, we should create an issue and add a link to it here.

The comment wasn't addressed but the thread was resolved.

It was, I just never linked the issue to the PR. #23672

jorgehermo9 · 2025-08-04T17:54:53Z

I didn't have any problem while running those tests on linux, could you point me some reproducible example, CI fail or something?

It is very weird that we have this problem with next_addr_any as it reuses the same functionality of next_addr. Does this happen with other tests that use next_addr?

pront · 2025-08-04T17:59:04Z

I didn't have any problem while running those tests on linux, could you point me some reproducible example, CI fail or something?

It is very weird that we have this problem with next_addr_any as it reuses the same functionality of next_addr. Does this happen with other tests that use next_addr?

I wonder if the utility itself hits a race condition.

thomasqueirozb · 2025-08-04T18:00:51Z

could you point me some reproducible example, CI fail or something?

This is not failing on CI since CI is using cargo nextest. However, if you run cargo test instead off of master right now on linux you will see the it hang on a multicast test

It is very weird that we have this problem with next_addr_any as it reuses the same functionality of next_addr. Does this happen with other tests that use next_addr?

Not sure. I think we're hitting this issue since next_addr_for_ip is trying to use the same port for 2 tests running on separate threads but I could be wrong

jorgehermo9 · 2025-08-04T19:00:08Z

This is not failing on CI since CI is using cargo nextest. However, if you run cargo test instead off of master right now on linux you will see the it hang on a multicast test

I can't reproduce it, every test is working for me. Executed it a few times and didn't fail... Running it on Linux 6.15.6-arch1-1

If the problem is the utility, maybe we should include the lock in lib/portpicker/src/lib.rs?

The usual way of atomically bind to a port is using the port number 0. But the portpicker lib ensures that both the udp and tcp port are free. Usually, we don't need both ports so a refactor to use port number 0 is feasible... I dug into that code when doing those multicast tests, but preferred to keep using next_addr_any.

Binding with a port number of 0 will request that the OS assigns a port to this listener. The port allocated can be queried via the TcpListener::local_addr method.

https://doc.rust-lang.org/std/net/struct.TcpListener.html#method.bind

However, I'm hesitant of fixing it the way you did, with the lock placed there inside the tests. Maybe we should add a test_util primitive that both gets the address with next_addr_for_ip and binds to the socket atomically with that global lock.

jorgehermo9 · 2025-08-04T19:09:18Z

Alternative, I think we should have next_addr_any, next_addr and next_addr_for_ip return the same lock guard you are using here alongside with the SocketAddr. this way, the caller can get any behaviour atomically and only one test that calls next_addr would be running at the same time

Something like

    static ADDR_LOCK: LazyLock<Mutex<()>> = LazyLock::new(|| Mutex::new(()));
pub async fn next_addr_for_ip(ip: IpAddr) -> (MutexGuard,SocketAddr) {
    let guard = ADDR_LOCK.lock().await;
    let port = pick_unused_port(ip);
    (guard,SocketAddr::new(ip, port))
}

But I think tokio's MutexGuard contains references so I'm not sure if you can return the guard like this

We can use instead lock_owned https://docs.rs/tokio/latest/tokio/sync/struct.Mutex.html#method.lock_owned to return that guard

src/sources/socket/mod.rs

thomasqueirozb · 2025-08-27T20:25:42Z

This is not failing on CI since CI is using cargo nextest. However, if you run cargo test instead off of master right now on linux you will see the it hang on a multicast test
I can't reproduce it, every test is working for me. Executed it a few times and didn't fail... Running it on Linux 6.15.6-arch1-1

Sorry, I thought the issue was reproducible in Linux but it isn't. Going to run the test suite on Linux before merging this just to make sure there are no issues

src/sources/socket/mod.rs

pront

Looks good to me. We can introduce a generic test coordinator component in the future to clean this up.

Waiting for an ubuntu run...

src/sources/socket/mod.rs

This reverts commit eed7a3878a5c0c74b3e759c8c470d42b353964d7.

thomasqueirozb · 2025-08-28T20:42:00Z

cargo test --all still fails but that's a fight for another day. Merging

pront · 2025-09-03T18:38:45Z

cargo test --all still fails but that's a fight for another day. Merging

Worth creating an issue so we can keep track of it. And as a FYI to the community.

Fix tests failing with cargo test

ee494f8

thomasqueirozb requested a review from a team as a code owner August 4, 2025 17:45

github-actions bot added the domain: sources Anything related to the Vector's sources label Aug 4, 2025

thomasqueirozb changed the title ~~fix(tests): Make tests pass with cargo test~~ fix(tests): Make tests pass with cargo test Aug 4, 2025

thomasqueirozb added the no-changelog Changes in this PR do not need user-facing explanations in the release changelog label Aug 4, 2025

pront reviewed Aug 4, 2025

View reviewed changes

src/sources/socket/mod.rs Show resolved Hide resolved

pront reviewed Aug 4, 2025

View reviewed changes

Use tokio Mutex instead

23d2bd9

thomasqueirozb force-pushed the fix-cargo-test branch from 3fd15d6 to 23d2bd9 Compare August 4, 2025 18:21

thomasqueirozb added 2 commits August 26, 2025 16:21

Merge branch 'master' into fix-cargo-test

e24d667

Implement global lock as suggested in PR review

a319dfb

thomasqueirozb commented Aug 27, 2025

View reviewed changes

src/sources/socket/mod.rs Show resolved Hide resolved

thomasqueirozb added 2 commits August 27, 2025 13:26

Fix clippy

cfceff5

Drop guard after binding

e80e378

thomasqueirozb requested review from jorgehermo9 and pront August 27, 2025 20:24

pront reviewed Aug 28, 2025

View reviewed changes

src/sources/socket/mod.rs Show resolved Hide resolved

Merge branch 'master' into fix-cargo-test

b728101

pront approved these changes Aug 28, 2025

View reviewed changes

src/sources/socket/mod.rs Outdated Show resolved Hide resolved

thomasqueirozb added 4 commits August 28, 2025 15:54

Rename wait_for_tcp->wait_for_tcp_and_release

8483792

Lock when using log_schema in tests

0dae3bb

Lock in tracing-limit's tests due to tracing::subcriber::with_default

7e1285b

Revert "Lock when using log_schema in tests"

7135568

This reverts commit eed7a3878a5c0c74b3e759c8c470d42b353964d7.

thomasqueirozb enabled auto-merge August 28, 2025 20:42

thomasqueirozb added this pull request to the merge queue Aug 28, 2025

Merged via the queue into master with commit e071b23 Aug 28, 2025
100 checks passed

thomasqueirozb deleted the fix-cargo-test branch August 28, 2025 21:19

thomasqueirozb mentioned this pull request Aug 29, 2025

sources::exec::tests::test_run_command_linux disabled on macos #23672

Open

thomasqueirozb mentioned this pull request Sep 3, 2025

cargo test --all fails to run on master #23718

Open

fix(tests): Make tests pass with cargo test #23520

fix(tests): Make tests pass with cargo test #23520

Uh oh!

Conversation

thomasqueirozb commented Aug 4, 2025

Summary

Vector configuration

How did you test this PR?

Change Type

Is this a breaking change?

Does this PR include user facing changes?

References

Notes

Uh oh!

Uh oh!

thomasqueirozb commented Aug 4, 2025

Uh oh!

pront Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

thomasqueirozb Aug 4, 2025

Choose a reason for hiding this comment

Uh oh!

pront Aug 28, 2025

Choose a reason for hiding this comment

Uh oh!

pront Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

thomasqueirozb Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

jorgehermo9 commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pront commented Aug 4, 2025

Uh oh!

thomasqueirozb commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgehermo9 commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jorgehermo9 commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thomasqueirozb commented Aug 27, 2025

Uh oh!

Uh oh!

pront left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomasqueirozb commented Aug 28, 2025

Uh oh!

Uh oh!

pront commented Sep 3, 2025

Uh oh!

Uh oh!

fix(tests): Make tests pass with `cargo test` #23520

fix(tests): Make tests pass with `cargo test` #23520

jorgehermo9 commented Aug 4, 2025 •

edited

Loading

thomasqueirozb commented Aug 4, 2025 •

edited

Loading

jorgehermo9 commented Aug 4, 2025 •

edited

Loading

jorgehermo9 commented Aug 4, 2025 •

edited

Loading