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

Increase test timeout from 1s -> 10s #672

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DavisVaughan
Copy link
Contributor

@DavisVaughan DavisVaughan commented Jan 23, 2025

I've been exploring nextest a bit for ark, and I would get some consistent false positive timeouts with a 1s timeout. I think having multiple processes running at once causes some kind of contention, so we need a longer timeout. With this one change (and after #669) I can run cargo nextest run on my mac without any issues.

I don't think having a big timeout here is that big a deal, we aren't trying to ensure its fast, we just dont want it to get hung.

I would really like to look into nextest as I think it will help us simplify our test infra quite a bit (no management of locks or global state or needing to worry about not cross contaminating the test R session, because each test gets its own R session). It looks like we can just switch to it if we want to with little extra effort, then start simplifying

@DavisVaughan DavisVaughan requested a review from lionel- January 23, 2025 21:10
@DavisVaughan
Copy link
Contributor Author

DavisVaughan commented Jan 23, 2025

Is the world just slow today? Lots of fun with timeouts, unrelated to nextest. But these also look like false alarms so I'll just up these too.

thread 'test_live_updates' panicked at crates/ark/tests/data_explorer.rs:1084:86:
called `Result::unwrap()` on an `Err` value: Timeout

Again, the test is not about speed, it's just about blowing up at some point when it looks like we are really hung
@DavisVaughan
Copy link
Contributor Author

DavisVaughan commented Jan 23, 2025

thread 'lsp::completions::sources::composite::call::tests::test_completions_after_user_types_part_of_an_argument_name' panicked at core\src\panicking.rs:223:5:
panic in a function that cannot unwind

this seems to reproduce on the windows runner (on main too, going back a few commits) but I have no idea why. I can't reproduce on my own windows machine.

@DavisVaughan
Copy link
Contributor Author

This one popped up again on the mac runner, but #673 should fix it

thread 'test_env_vars' panicked at crates/amalthea/src/fixtures/dummy_frontend.rs:195:9:
assertion failed: `Status(JupyterMessage { zmq_identities: [], header: JupyterHeader { msg_id: "24ea361b-b116-462e-a868-47d503ae01be", session: "5ccdaa85-4179-49b6-b8e2-97b484081e7e", username: "kernel", date: "2025-01-23T21:23:15.387272+00:00", msg_type: "status", version: "5.3" }, parent_header: None, content: KernelStatus { execution_state: Starting } })` does not match `Message::Welcome(data)`

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.

1 participant