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

Adding test for a valid config update after an invalid config #750

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fabian-moessner
Copy link
Collaborator

No description provided.

@fabian-moessner fabian-moessner marked this pull request as ready for review January 30, 2025 14:56
Copy link
Collaborator

@ekneg54 ekneg54 left a comment

Choose a reason for hiding this comment

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

This does not reproduces the failure. Please have a look deeper.
unittests for the refresh interval can be found in tests/unit/test_runner.py. Consider starting there to go deeper into testing and bug reproduction.

wait_for_output(proc, "Config refresh interval is set to: 5 seconds", test_timeout=5)

config.config_refresh_interval = None
config.version = "invalid"
Copy link
Collaborator

Choose a reason for hiding this comment

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

as described in

version: str = field(
validator=validators.instance_of(str), converter=str, default="unset", eq=True
the version attribute is of type "str". So setting it to the string "invalid" doesn't result in an invalid configuration.

config.version = "valid_again"
config_path.write_text(config.as_json())
wait_for_output(proc, "Successfully reloaded configuration", test_timeout=10)
wait_for_output(proc, "Config refresh interval is set to: 4 seconds", test_timeout=5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

as defined here

Logprep/logprep/runner.py

Lines 120 to 129 in 0068804

@_config_refresh_interval.setter
def _config_refresh_interval(self, value: int | None) -> None:
"""Set the configuration refresh interval in seconds."""
if value is None:
self._configuration.config_refresh_interval = None
elif value <= 5:
self._configuration.config_refresh_interval = 5
else:
self._configuration.config_refresh_interval = value
the minimum for a config refresh interval should be 5 Seconds. If you are able to set it to 4 Seconds we might have a new bug.

config.config_refresh_interval = None
config.version = "invalid"
config_path.write_text(config.as_json())
time.sleep(3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

sleep actions in test leads to flaking tests because you never know if a test run on a slower system does not needs some seconds more. It is better to wait for some condition in the logs for example.

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.

2 participants