[DBMON-6334] Add ClickHouse unit tests to track all config default value changes#23057
[DBMON-6334] Add ClickHouse unit tests to track all config default value changes#23057sangeetashivaji wants to merge 2 commits intomasterfrom
Conversation
|
This PR does not modify any files shipped with the agent. To help streamline the release process, please consider adding the |
sangeetashivaji
force-pushed
the
sangeeta.shivajirao/test_config_defaults
branch
from
0a64704 to
3927649
Compare
sangeetashivaji
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files🚀 New features to boost your workflow:
|
sangeetashivaji
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 39276492a5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for key, expected_value in expected_dict.items(): | ||
| try: | ||
| actual_value = getattr(actual_obj, key) |
There was a problem hiding this comment.
The nested comparison only loops over expected_dict.items() and never checks for extra attributes present on the actual nested model, so new fields added under objects like query_metrics or query_samples can slip in without failing this test. In that scenario, missing_from_expected still passes (top-level field unchanged) and _compare_nested_object also passes, which breaks the stated guarantee that every default change is explicitly tracked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed: added nested fields check too
Example: added check for nested fields in Query metrics
'query_metrics': {
'enabled': True,
'collection_interval': 10,
'run_sync': False,
'full_statement_text_cache_max_size': 10000,
'full_statement_text_samples_per_hour_per_query': 1,
},
1 participant
What does this PR do?
The goal of this PR is to ensure our default config values remain the same. We're now explicitly asserting against all default rendered config values from InstanceConfig. Any changes to our hardcoded expected defaults will trigger a test failure with a clear message as to why it's failing and steps to take to mitigate. Similarly if new config values are added and they aren't asserted against in this test it will trigger a failure and require fixing up.
Example failure message when a new testConfig value is added but wasn't updated in the test
comparing the defaults from the previous inline config here vs the new ClickHouse DBM support PR
Motivation
The goal of this change is to ensure our config default values don't change unexpectedly or accidentally overtime to avoid regressions.
Tests:
Config defaults tests
Review checklist (to be filled by reviewers)
qa/skip-qalabel if the PR doesn't need to be tested during QA.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged