Add unit tests for previously-untested modules; fix DynamicConfig.get_config()#858
Open
vgvoleg wants to merge 2 commits into
Open
Add unit tests for previously-untested modules; fix DynamicConfig.get_config()#858vgvoleg wants to merge 2 commits into
vgvoleg wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a parsing bug in the draft Dynamic Config client and adds a new set of unit tests for several client wrapper modules, with CI updates to include the new unit-test locations in coverage runs.
Changes:
- Fix
_wrap_dynamic_config()to treatidentity/configas singular protobuf fields (no[0]indexing). - Add unit tests under
tests/unit/covering scripting, operation, import/export wrappers, global settings, and dynamic config wrappers. - Update the GitHub Actions coverage workflow to split “unit” vs “integration” coverage runs around
tests/unit/.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
ydb/draft/dynamic_config.py |
Fixes DynamicConfigClient.get_config() response wrapping (singular field access). |
tests/unit/test_scripting.py |
New unit tests for scripting request factories, wrappers, and dispatch behavior. |
tests/unit/test_operation.py |
New unit tests for operation request/response helpers and client/operation dispatch. |
tests/unit/test_import_client.py |
New unit tests for import-from-S3 settings/request factory and dispatch/wrappers. |
tests/unit/test_global_settings.py |
New unit tests for global settings toggles and warning behavior. |
tests/unit/test_export.py |
New unit tests for export settings/request factories and dispatch/wrappers. |
tests/unit/test_dynamic_config.py |
New unit tests covering dynamic config request factories, wrappers, and dispatch (incl. the fixed path). |
tests/unit/__init__.py |
Adds tests.unit package marker for the new unit-test suite. |
CHANGELOG.md |
Adds a user-facing entry for the DynamicConfigClient.get_config() bug fix. |
.github/workflows/tests.yaml |
Adjusts coverage jobs to run tests/unit/ under “unit” and ignore them under “integration”. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
72
to
+74
| def _wrap_dynamic_config(config_pb, dynamic_config_cls=None, *args, **kwargs): | ||
| dynamic_config_cls = DynamicConfig if dynamic_config_cls is None else dynamic_config_cls | ||
| return dynamic_config_cls( | ||
| config_pb.identity[0].version, config_pb.identity[0].cluster, config_pb.config[0], *args, **kwargs | ||
| ) | ||
| return dynamic_config_cls(config_pb.identity.version, config_pb.identity.cluster, config_pb.config, *args, **kwargs) |
Comment on lines
+92
to
+95
| - name: Unit tests with coverage | ||
| env: | ||
| COVERAGE_CORE: sysmon | ||
| run: tox -e cov -- ydb | ||
| run: tox -e cov -- ydb tests/unit |
Cover export, import, scripting, operation, dynamic_config and global_settings with unit tests (tests/unit/, no server needed) — all six modules to 100%. Fixes get_config() raising TypeError from indexing singular protobuf fields. CI counts tests/unit/ as unit.
vgvoleg
force-pushed
the
coverage-client-wrappers
branch
from
320ef92 to
9b82c88
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #858 +/- ##
==========================================
+ Coverage 81.30% 84.09% +2.78%
==========================================
Files 94 93 -1
Lines 12162 12131 -31
Branches 1194 1194
==========================================
+ Hits 9888 10201 +313
+ Misses 1809 1470 -339
+ Partials 465 460 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…ts, opentelemetry The grpcwrapper ydb_scheme Entry dataclass was orphaned since it was added — topic describe uses ydb/scheme.py instead, and it was never imported. Drop it and cover tracing, topic reader events and the opentelemetry entrypoint with unit tests (all three to 100%).
vgvoleg
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds unit tests (no server required,
tests/unit/) for previously-untested modules, taking each to 100%:export.pyimport_client.pyscripting.pyoperation.pydraft/dynamic_config.pyglobal_settings.pytracing.py_topic_reader/events.pyopentelemetry/__init__.pyBug fix:
DynamicConfigClient.get_config()raisedTypeError—_wrap_dynamic_configindexed the singularidentity/configprotobuf fields as if repeated ([0]). Reverted to attribute access; the new tests cover this path.Dead code removed:
ydb/_grpc/grpcwrapper/ydb_scheme.py— an orphaned scheme-entry dataclass, never imported by anything (topic describe usesydb/scheme.py).Tests live in
tests/unit/(out of the shippedydb/package).