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

Make ConfigChangeObserverTest poll for change #873

Open
pont-us opened this issue Aug 3, 2023 · 1 comment
Open

Make ConfigChangeObserverTest poll for change #873

pont-us opened this issue Aug 3, 2023 · 1 comment
Assignees

Comments

@pont-us
Copy link
Member

pont-us commented Aug 3, 2023

Describe the bug
A clear and concise description of what the bug is.

ConfigChangeObserverTest.test_config_change_observer points a ConfigChangeObserver at a configuration, changes the configuration, then immediately checks that ConfigChangeObserver noticed the change. There isn't always time for this to happen. Sometimes the check is done before ConfigChangeObserver has had time to reach its next polling interval and load the new configuration from the file. This results in a spurious test failure.

To Reproduce
Steps to reproduce the behaviour:

Run the ConfigChangeObserverTest.test_config_change_observer repeatedly. Occasionally it fails. (Frequency of failures dependent on system configuration and current load.)

Expected behaviour
The test should pass every time.

Additional context
The simple but inefficient way to fix this is to introduce a fixed delay the the test between updating the configuration file and checking that the change has been registered. However, I have discovered that occasionally (rarely) this can take up to two seconds, and it would be inefficient to slow down every test run by two seconds for these rare cases. The test should poll for the change so it doesn't waste any time, with a fall-back time-out of two seconds or so, to catch genuine failures where ConfigChangeListener really doesn't register it.

@pont-us pont-us self-assigned this Aug 3, 2023
@pont-us pont-us assigned ruchimotwaniBC and unassigned pont-us Oct 4, 2023
@forman
Copy link
Member

forman commented Jan 18, 2024

I don't like time.sleep solutions as they

  • don't make the (test) code more deterministic and
  • slow down test runs.

Consider mocking or testing at a different call level.

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

3 participants