Skip to content

(#3843) bugfix: increase timeout to 1 second in TestConfigFile #3849

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
Closed
wants to merge 2 commits into from
Closed

(#3843) bugfix: increase timeout to 1 second in TestConfigFile #3849

wants to merge 2 commits into from

Conversation

mattkae
Copy link
Contributor

@mattkae mattkae commented Mar 24, 2025 •
edited
Loading

fixes #3843

So this test (and its kin) rely on a bit of a timing. The order of events is:

  1. Start monitoring the config file on the main thread. This enqueues a task onto the fd monitor thread.
  2. Write the config file on the main thread.
  3. Wait on the config to be written on main thread (timeout is 10ms)
  4. Expect that the fd thread fires in the meantime telling us that the file has changed

Steps 3 and 4 currently have to happen within 10ms. This is likely to happen more often than not. However, if a machine has a slow moment, this could be too little time. My recommendation here is to increase that timeout.

What's new?

  • Increase test timeout from 10ms to 1s to increase chance of success on slow machines

@mattkae mattkae changed the title (#3843) bugfix: initial idea is to make sure we remove the watch to not leave dangling queued events (potentially) (#3843) bugfix: increase timeout in TestConfigFile and no longer leaving dangling queued events Mar 24, 2025
@mattkae mattkae requested a review from AlanGriffiths March 24, 2025 14:50
@mattkae mattkae marked this pull request as ready for review March 24, 2025 14:52
@mattkae mattkae requested a review from a team as a code owner March 24, 2025 14:52
RAOF
RAOF reviewed Mar 25, 2025
View reviewed changes
Copy link
Contributor

@RAOF RAOF left a comment

Choose a reason for hiding this comment

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

The timeout change seems sensible; it only extends the duration of failing test runs.

Question on the other bit.

@mattkae mattkae changed the title (#3843) bugfix: increase timeout in TestConfigFile and no longer leaving dangling queued events (#3843) bugfix: increase timeout to 10 seconds in TestConfigFile Mar 25, 2025
@mattkae mattkae force-pushed the bugfix/config_watching branch from dfe777a to 883d414 Compare March 25, 2025 12:18
@mattkae mattkae requested a review from RAOF March 25, 2025 12:18
AlanGriffiths
AlanGriffiths requested changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

This affects both tests that expect a load and those that do not expect a load. Extending the runtime of, e.g., TestConfigFile.with_no_reloading_when_a_file_is_written_nothing_is_loaded to 10s is excessive.

Maybe we need two wait periods? (And 10s seems overkill anyway)

@mattkae mattkae force-pushed the bugfix/config_watching branch from 883d414 to a60a36d Compare March 25, 2025 13:19
@mattkae
Copy link
Contributor Author

mattkae commented Mar 25, 2025

This affects both tests that expect a load and those that do not expect a load. Extending the runtime of, e.g., TestConfigFile.with_no_reloading_when_a_file_is_written_nothing_is_loaded to 10s is excessive.

Maybe we need two wait periods? (And 10s seems overkill anyway)

I made the wait period 1 second instead. If we have two wait periods, that would be unfair to one of the wait periods (i.e. we're giving some tests a fighting chance to see a file change event, while other tests technically might not have that same chance)

@mattkae mattkae changed the title (#3843) bugfix: increase timeout to 10 seconds in TestConfigFile (#3843) bugfix: increase timeout to 1 second in TestConfigFile Mar 25, 2025
@mattkae mattkae requested a review from AlanGriffiths March 25, 2025 13:21
@mattkae mattkae force-pushed the bugfix/config_watching branch from a60a36d to d56bbb7 Compare March 25, 2025 14:21
AlanGriffiths
AlanGriffiths reviewed Mar 25, 2025
View reviewed changes
AlanGriffiths
AlanGriffiths previously approved these changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@AlanGriffiths AlanGriffiths left a comment

Choose a reason for hiding this comment

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

I'd tone it down a bit, but won't block

tests/miral/config_file.cpp Outdated
{
std::cerr << "wait_for_load() timed out" << std::endl;
}
}

void wait_to_ensure_no_load()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this name is a bit verbose? wait_for_nothing()?

@mattkae mattkae force-pushed the bugfix/config_watching branch from d56bbb7 to ee744fc Compare March 25, 2025 17:53
@AlanGriffiths
Copy link
Contributor

😞 this wasn't enough (but that does makes me wonder if the timeout is the real problem here)

2025-03-25T17:58:49.7888697Z wait_for_load() timed out
2025-03-25T17:58:49.7889122Z [2025-03-25 17:57:29.984231] <information> input-hub: Device configuration: mousekey-pointer, capabilities={pointer}
2025-03-25T17:58:49.7889636Z [2025-03-25 17:57:29.984400] <information> mirserver: Stopping
2025-03-25T17:58:49.7889972Z ../../../tests/miral/config_file.cpp:303: Failure
2025-03-25T17:58:49.7890433Z Actual function call count doesn't match EXPECT_CALL(*this, load(_, xdg_conf_home/config_file))...
2025-03-25T17:58:49.7890869Z Expected: to be called once
2025-03-25T17:58:49.7891139Z Actual: never called - unsatisfied and active
2025-03-25T17:58:49.7891343Z 
2025-03-25T17:58:49.7891781Z [ FAILED ] TestConfigFile.with_reload_on_change_after_a_config_in_xdg_conf_dir0_is_loaded_a_new_config_in_xdg_conf_home_is_loaded (888 ms)

From this run

@mattkae
Copy link
Contributor Author

mattkae commented Mar 26, 2025

😞 this wasn't enough (but that does makes me wonder if the timeout is the real problem here)

2025-03-25T17:58:49.7888697Z wait_for_load() timed out
2025-03-25T17:58:49.7889122Z [2025-03-25 17:57:29.984231] <information> input-hub: Device configuration: mousekey-pointer, capabilities={pointer}
2025-03-25T17:58:49.7889636Z [2025-03-25 17:57:29.984400] <information> mirserver: Stopping
2025-03-25T17:58:49.7889972Z ../../../tests/miral/config_file.cpp:303: Failure
2025-03-25T17:58:49.7890433Z Actual function call count doesn't match EXPECT_CALL(*this, load(_, xdg_conf_home/config_file))...
2025-03-25T17:58:49.7890869Z Expected: to be called once
2025-03-25T17:58:49.7891139Z Actual: never called - unsatisfied and active
2025-03-25T17:58:49.7891343Z 
2025-03-25T17:58:49.7891781Z [ FAILED ] TestConfigFile.with_reload_on_change_after_a_config_in_xdg_conf_dir0_is_loaded_a_new_config_in_xdg_conf_home_is_loaded (888 ms)

From this run

I saw that you marked this as "obsolete". Are you just saying that the link is "obsolete"? Or is the whole problem "obsolete"?

@mattkae mattkae requested a review from AlanGriffiths March 26, 2025 12:14
@AlanGriffiths
Copy link
Contributor

I saw that you marked this as "obsolete". Are you just saying that the link is "obsolete"? Or is the whole problem "obsolete"?

I marked my earlier "approval" as obsolete as the problem isn't fixed

@mattkae mattkae force-pushed the bugfix/config_watching branch from 853ac04 to 83298b9 Compare March 31, 2025 19:18
@mattkae mattkae closed this Apr 11, 2025
@mattkae mattkae deleted the bugfix/config_watching branch April 11, 2025 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RAOF RAOF Awaiting requested review from RAOF

@AlanGriffiths AlanGriffiths Awaiting requested review from AlanGriffiths

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unstable TestConfigFile. test
3 participants
@mattkae @AlanGriffiths @RAOF