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.

Sign up for GitHub

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

Jump to bottom

cli_send_* tests in zellij_server fail when running on too many threads #1929

Open
har7an opened this issue Nov 12, 2022 · 3 comments
Open

cli_send_* tests in zellij_server fail when running on too many threads #1929

har7an opened this issue Nov 12, 2022 · 3 comments

Comments

@har7an
Copy link
Contributor

har7an commented Nov 12, 2022

I noticed that the tests in zellij_server, when run from cargo make, started failing on my machine with one of the recent changes in main. I haven't yet bothered to track down the exact change that introduced this.

The symptom for me is that the following 16 tests inside zellij_server fail:

screen::screen_tests::send_cli_close_pane_action
screen::screen_tests::send_cli_close_tab_action
screen::screen_tests::send_cli_edit_action_with_default_parameters
screen::screen_tests::send_cli_edit_action_with_line_number
screen::screen_tests::send_cli_edit_action_with_split_direction
screen::screen_tests::send_cli_edit_scrollback_action
screen::screen_tests::send_cli_focus_next_pane_action
screen::screen_tests::send_cli_focus_previous_pane_action
screen::screen_tests::send_cli_goto_tab_action
screen::screen_tests::send_cli_half_page_scroll_down_action
screen::screen_tests::send_cli_half_page_scroll_up_action
screen::screen_tests::send_cli_move_focus_or_tab_pane_action
screen::screen_tests::send_cli_move_focus_pane_action
screen::screen_tests::send_cli_move_pane_action
screen::screen_tests::send_cli_new_pane_action_with_command_and_cwd
screen::screen_tests::send_cli_new_pane_action_with_default_parameters

When limiting the number of CPU threads to use with e.g. taskset, less tests will fail. When giving 6 threads to cargo make, all tests pass on my machine. This also explains why the CI doesn't fail, because IIRC that only has 3 threads available for x86 Ubuntu workers. The command I have to use is:

taskset -c 0-5 cargo make ...

As far as I could determine, this doesn't seem to be an error from shared global state. However, all these tests use insta to compare snapshots, and I don't know how insta works, so it may well be caused by races in global states of some sort. I tried the following to alleviate race conditions:

  • Use rusty_fork to fork out the tests into a more isolated environment
  • Add a global Mutex to the tests so simulating the sending from CLI doesn't happen in parallel any longer

None of these made the problems go away. One solution would be to limit the number of test threads in cargo make, but I'm wondering whether I'm the only person experiencing this? I also tried cargo clean repeatedly in case it was caused by some old artifacts messing with the tests, but that hasn't changed the situation for me.
@imsnif
Copy link
Member

imsnif commented Nov 12, 2022

What are the failures you're getting?

@har7an
Copy link
Contributor Author

har7an commented Nov 12, 2022

Usually looks something like this:

---- screen::screen_tests::send_cli_focus_previous_pane_action stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Differences ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: zellij-server/src/./unit/snapshots/zellij_server__screen__screen_tests__send_cli_focus_previous_pane_action.snap
Snapshot: send_cli_focus_previous_pane_action
Source: zellij-server/src/unit/screen_tests.rs:1061
-old snapshot
+new results
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
format!("{}", snapshot_count)
────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────
    0       │-Some((1, 1))
          0 │+0
────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`

The "new snapshot" is always "0".

@imsnif
Copy link
Member

imsnif commented Nov 13, 2022

The "new snapshot" is always "0".

This is a race. My guess would be that we're getting one less snapshot because we quit before the last render/update or some such. It's not significant for the test and can be fixed by only asserting the specific event we're looking for was received (in this case it would be the focus previous pane event).

I'm pretty sure all of these are something like that.

@f3fora f3fora mentioned this issue Dec 7, 2022
Open
3 tasks
@har7an har7an mentioned this issue Dec 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@imsnif @har7an