Skip to content

🧪 [testing improvement: add tests for dq_report_results_core value objects]#2610

Open
SatoryKono wants to merge 1 commit intomainfrom
jules-7876919980544846177-6a99eb59
Open

🧪 [testing improvement: add tests for dq_report_results_core value objects]#2610
SatoryKono wants to merge 1 commit intomainfrom
jules-7876919980544846177-6a99eb59

Conversation

@SatoryKono
Copy link
Copy Markdown
Owner

@SatoryKono SatoryKono commented Apr 2, 2026

Implemented unit tests for DQ report core result value objects, focusing on list-to-tuple conversion for immutability in post_init methods. Classes covered: TypeConformanceResult, SchemaSnapshotResult, EncodingValidationResult, CategoricalDistribution, and SchemaDriftResult. All 14498 unit tests passed.


PR created automatically by Jules for task 7876919980544846177 started by @SatoryKono

Summary by CodeRabbit

Tests

  • Added comprehensive unit test coverage for data quality result value objects, validating proper object initialization, field type conversions, and status handling across multiple core components.

…jects]

Implemented unit tests for DQ report core result value objects in
src/bioetl/domain/value_objects/dq_report_results_core.py.

Key improvements:
- Verified list-to-tuple conversion for immutability in __post_init__ methods
- Added coverage for TypeConformanceResult, SchemaSnapshotResult,
  EncodingValidationResult, CategoricalDistribution, and SchemaDriftResult
- Added basic instantiation tests for UniquenessResult

These tests ensure that the domain layer maintains strict immutability
even when initialized with mutable list arguments. Verified with over 14k unit tests.

Co-authored-by: SatoryKono <13055362+SatoryKono@users.noreply.github.com>
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

📝 Walkthrough

Walkthrough

A new unit test module was added to verify that DQ result value objects correctly convert constructor-provided list fields to tuples during initialization and retain scalar attributes as expected across six different result classes.

Changes

Cohort / File(s) Summary
DQ Result Value Object Tests
tests/unit/domain/value_objects/test_dq_report_results_core.py
New test module covering six DQ result value objects (SchemaSnapshotResult, EncodingValidationResult, UniquenessResult, TypeConformanceResult, CategoricalDistribution, SchemaDriftResult), verifying list-to-tuple field conversions and scalar attribute retention during object initialization.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 The tests now dance where value objects dwell,
Tuples and lists behave oh so well!
From lists to tuples, conversions take flight,
Our DQ checks gleam with test-driven light! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is incomplete. While it mentions the test implementation and classes covered, it lacks structured sections (Summary, Changes, Type, Affected layers, Test plan, Checklist) required by the template. Restructure the description to follow the template with sections for Summary, Changes, Type checkboxes, Affected layers, Test plan, and Checklist items.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: adding unit tests for dq_report_results_core value objects.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jules-7876919980544846177-6a99eb59

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
tests/unit/domain/value_objects/test_dq_report_results_core.py (2)

23-35: Strengthen immutability checks by mutating the original input lists after construction.

Right now these tests verify conversion shape, but not alias-safety against post-construction mutation of caller-owned lists. Add a mutation step and assert stored tuples remain unchanged.

Suggested pattern (apply similarly across all list→tuple tests)
 def test_list_to_tuple_conversion(self) -> None:
-    result = SchemaSnapshotResult(
+    new_fields = ["f1", "f2"]
+    missing_fields = ["f3"]
+    result = SchemaSnapshotResult(
         fields_detected=5,
         schema={"col1": "int"},
-        new_fields_since_last_run=["f1", "f2"],  # type: ignore[arg-type]
-        missing_fields_since_last_run=["f3"],  # type: ignore[arg-type]
+        new_fields_since_last_run=new_fields,  # type: ignore[arg-type]
+        missing_fields_since_last_run=missing_fields,  # type: ignore[arg-type]
     )
+    new_fields.append("f4")
+    missing_fields.append("f5")
     assert isinstance(result.new_fields_since_last_run, tuple)
     assert result.new_fields_since_last_run == ("f1", "f2")
     assert isinstance(result.missing_fields_since_last_run, tuple)
     assert result.missing_fields_since_last_run == ("f3",)

Also applies to: 41-49, 75-84, 90-98, 104-117

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/domain/value_objects/test_dq_report_results_core.py` around lines
23 - 35, Enhance the test_list_to_tuple_conversion test to verify alias-safety
by mutating the original input lists after constructing SchemaSnapshotResult and
asserting that SchemaSnapshotResult.new_fields_since_last_run and
.missing_fields_since_last_run remain unchanged tuples; specifically, in the
test (test_list_to_tuple_conversion) create mutable lists for
new_fields_since_last_run and missing_fields_since_last_run, pass them to
SchemaSnapshotResult, then append/remove items from those original lists and
assert the instance's attributes still equal ("f1","f2") and ("f3",)
respectively. Apply the same mutation-after-construction pattern to the other
list→tuple tests mentioned (lines covering the other test functions) to ensure
immutability across SchemaSnapshotResult conversions.

19-117: Consider parametrizing repeated list-to-tuple conversion tests to reduce duplication.

The conversion tests follow nearly identical structure; pytest.mark.parametrize (or shared helper) would make additions cheaper and reduce maintenance overhead.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/unit/domain/value_objects/test_dq_report_results_core.py` around lines
19 - 117, Multiple test methods duplicate the same "list-to-tuple conversion"
pattern; replace them with a single parametrized test using
pytest.mark.parametrize that iterates over cases (use the classes/constructors
and fields: SchemaSnapshotResult.new_fields_since_last_run,
SchemaSnapshotResult.missing_fields_since_last_run,
EncodingValidationResult.invalid_utf8_records, TypeConformanceResult.errors,
CategoricalDistribution.top_values, SchemaDriftResult.new_fields,
SchemaDriftResult.missing_fields, SchemaDriftResult.type_changes). For each
param include the constructor kwargs that pass a list for the target field and
the expected tuple, then assert isinstance(getattr(instance, field_name), tuple)
and equality to expected; keep UniquenessResult test separate. Update or remove
the individual test_list_to_tuple_conversion methods in
TestSchemaSnapshotResult, TestEncodingValidationResult,
TestTypeConformanceResult, TestCategoricalDistribution, and
TestSchemaDriftResult to use the new parametrized test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/unit/domain/value_objects/test_dq_report_results_core.py`:
- Around line 23-35: Enhance the test_list_to_tuple_conversion test to verify
alias-safety by mutating the original input lists after constructing
SchemaSnapshotResult and asserting that
SchemaSnapshotResult.new_fields_since_last_run and
.missing_fields_since_last_run remain unchanged tuples; specifically, in the
test (test_list_to_tuple_conversion) create mutable lists for
new_fields_since_last_run and missing_fields_since_last_run, pass them to
SchemaSnapshotResult, then append/remove items from those original lists and
assert the instance's attributes still equal ("f1","f2") and ("f3",)
respectively. Apply the same mutation-after-construction pattern to the other
list→tuple tests mentioned (lines covering the other test functions) to ensure
immutability across SchemaSnapshotResult conversions.
- Around line 19-117: Multiple test methods duplicate the same "list-to-tuple
conversion" pattern; replace them with a single parametrized test using
pytest.mark.parametrize that iterates over cases (use the classes/constructors
and fields: SchemaSnapshotResult.new_fields_since_last_run,
SchemaSnapshotResult.missing_fields_since_last_run,
EncodingValidationResult.invalid_utf8_records, TypeConformanceResult.errors,
CategoricalDistribution.top_values, SchemaDriftResult.new_fields,
SchemaDriftResult.missing_fields, SchemaDriftResult.type_changes). For each
param include the constructor kwargs that pass a list for the target field and
the expected tuple, then assert isinstance(getattr(instance, field_name), tuple)
and equality to expected; keep UniquenessResult test separate. Update or remove
the individual test_list_to_tuple_conversion methods in
TestSchemaSnapshotResult, TestEncodingValidationResult,
TestTypeConformanceResult, TestCategoricalDistribution, and
TestSchemaDriftResult to use the new parametrized test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 93b00768-704a-4464-9eee-7e10c9e0ba09

📥 Commits

Reviewing files that changed from the base of the PR and between 214d735 and 98543cb.

📒 Files selected for processing (1)
  • tests/unit/domain/value_objects/test_dq_report_results_core.py

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.

1 participant