test(consistent): curate DPA3 descriptor matrix#5390
test(consistent): curate DPA3 descriptor matrix#5390njzjz-bot wants to merge 4 commits intodeepmodeling:masterfrom
Conversation
…case parameterization pattern from deepmodeling#5378 to the\nDPA3 descriptor consistent tests so the coverage stays reviewable\nwithout relying on a Cartesian-product explosion.\n\nAuthored by OpenClaw (model: gpt-5.4)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced explicit test parameter matrices in DPA3 tests with programmatic case builders and curated case sets via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
source/tests/consistent/descriptor/test_dpa3.py (1)
138-173: Consider keeping one DescriptorAPI curated case withadd_chg_spin_ebd=True.
DPA3_DESCRIPTOR_API_CASE_FIELDS = DPA3_CASE_FIELDS[:-1]drops this axis entirely. Keeping at least one API case withadd_chg_spin_ebd=Truewould better protect conditional descriptor serialization/config paths.Based on learnings: in
deepmd/pd/model/descriptor/dpa3.py, optional serialization fields are gated byadd_chg_spin_ebd.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/tests/consistent/descriptor/test_dpa3.py` around lines 138 - 173, The curated API cases drop the last axis via DPA3_DESCRIPTOR_API_CASE_FIELDS = DPA3_CASE_FIELDS[:-1], so add at least one curated entry that sets add_chg_spin_ebd=True to exercise the conditional serialization gated by that flag; update DPA3_DESCRIPTOR_API_CURATED_CASES by inserting a dpa3_descriptor_api_case(add_chg_spin_ebd=True) (or add it into the existing mixed high-risk case) so the dpa3_descriptor_api_case function (which builds cases from DPA3_BASELINE_CASE and DPA3_DESCRIPTOR_API_CASE_FIELDS) covers the add_chg_spin_ebd path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/tests/consistent/descriptor/test_dpa3.py`:
- Around line 103-106: The dpa3_case builder should validate override keys and
avoid sharing mutable baseline state: create a deep copy of DPA3_BASELINE_CASE
(to prevent cross-case mutation), check that all keys in overrides are present
in DPA3_BASELINE_CASE and raise a clear error on any unknown keys (to catch
typos), then update the copied case with overrides and return the
tuple(case[field] for field in DPA3_CASE_FIELDS). Apply the same deep-copy +
unknown-key validation pattern to the other case-builder functions referenced
around lines 141-143.
- Around line 176-177: Several test methods (notably skip_pt, skip_pd, skip_dp,
skip_tf and others in TestDPA3) unpack many variables that are never used,
triggering RUF059; update the unpacked variable names to use a leading
underscore for intentionally unused variables (e.g., change update_residual_init
to _update_residual_init) in the tuple/list unpacking inside those methods and
any similar destructuring in TestDPA3/DescriptorTest helper calls so ruff no
longer flags the unused locals; keep the real used names unchanged and only
prefix the unused identifiers with underscores throughout those methods.
---
Nitpick comments:
In `@source/tests/consistent/descriptor/test_dpa3.py`:
- Around line 138-173: The curated API cases drop the last axis via
DPA3_DESCRIPTOR_API_CASE_FIELDS = DPA3_CASE_FIELDS[:-1], so add at least one
curated entry that sets add_chg_spin_ebd=True to exercise the conditional
serialization gated by that flag; update DPA3_DESCRIPTOR_API_CURATED_CASES by
inserting a dpa3_descriptor_api_case(add_chg_spin_ebd=True) (or add it into the
existing mixed high-risk case) so the dpa3_descriptor_api_case function (which
builds cases from DPA3_BASELINE_CASE and DPA3_DESCRIPTOR_API_CASE_FIELDS) covers
the add_chg_spin_ebd path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ff4959d0-4a28-4284-a11f-440883f777c6
📒 Files selected for processing (1)
source/tests/consistent/descriptor/test_dpa3.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5390 +/- ##
==========================================
- Coverage 80.33% 80.33% -0.01%
==========================================
Files 819 819
Lines 85356 85357 +1
Branches 4139 4140 +1
==========================================
Hits 68571 68571
- Misses 15509 15510 +1
Partials 1276 1276 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…d_chg_spin_ebd in the curated DPA3 descriptor API case\nmatrix so serialization coverage includes the conditional charge/spin\nembedding path noted in review.\n\nAuthored by OpenClaw (model: gpt-5.4)
|
Addressed. I updated the DPA3 descriptor API curated cases to keep — OpenClaw 2026.4.8 (model: gpt-5.4) |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
source/tests/consistent/descriptor/test_dpa3.py (1)
84-87:⚠️ Potential issue | 🟡 MinorHarden case builders against typoed override keys and baseline mutable-state leakage.
Line 103 and Line 141 accept any override keys, so misspelled toggles are silently ignored in the emitted tuple. Also, Line 86 keeps a mutable list in the shared baseline, which can leak across cases if mutated.
Suggested fix
+from copy import deepcopy + +def _build_dpa3_case(fields: tuple[str, ...], **overrides: Any) -> tuple: + unknown = set(overrides) - set(fields) + if unknown: + raise KeyError(f"Unknown DPA3 case override(s): {sorted(unknown)}") + case = deepcopy(DPA3_BASELINE_CASE) + case.update(overrides) + return tuple(case[field] for field in fields) + def dpa3_case(**overrides: Any) -> tuple: - case = DPA3_BASELINE_CASE | overrides - return tuple(case[field] for field in DPA3_CASE_FIELDS) + return _build_dpa3_case(DPA3_CASE_FIELDS, **overrides) @@ def dpa3_descriptor_api_case(**overrides: Any) -> tuple: - case = DPA3_BASELINE_CASE | overrides - return tuple(case[field] for field in DPA3_DESCRIPTOR_API_CASE_FIELDS) + return _build_dpa3_case(DPA3_DESCRIPTOR_API_CASE_FIELDS, **overrides)#!/bin/bash set -euo pipefail sed -n '84,145p' source/tests/consistent/descriptor/test_dpa3.py python - <<'PY' import ast, pathlib p = pathlib.Path("source/tests/consistent/descriptor/test_dpa3.py") src = p.read_text() mod = ast.parse(src) for fn in mod.body: if isinstance(fn, ast.FunctionDef) and fn.name in {"dpa3_case", "dpa3_descriptor_api_case"}: body = ast.get_source_segment(src, fn) print(f"{fn.name}: has_unknown_key_guard=", ("KeyError" in body or "unknown" in body)) print(f"{fn.name}: uses_baseline_union=", ("| overrides" in body)) PYAlso applies to: 103-105, 141-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/tests/consistent/descriptor/test_dpa3.py` around lines 84 - 87, DPA3 tests currently reuse a shared mutable DPA3_BASELINE_CASE and accept arbitrary override keys in dpa3_case and dpa3_descriptor_api_case; fix by defensively copying the baseline (e.g., create a fresh dict from DPA3_BASELINE_CASE and deep/copy mutable fields like "exclude_types") and validate overrides: compute an allowed_keys set from DPA3_BASELINE_CASE.keys() and raise KeyError for any key in overrides not in allowed_keys before applying them, then apply overrides to the copied baseline and proceed; reference DPA3_BASELINE_CASE, dpa3_case, and dpa3_descriptor_api_case when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@source/tests/consistent/descriptor/test_dpa3.py`:
- Around line 84-87: DPA3 tests currently reuse a shared mutable
DPA3_BASELINE_CASE and accept arbitrary override keys in dpa3_case and
dpa3_descriptor_api_case; fix by defensively copying the baseline (e.g., create
a fresh dict from DPA3_BASELINE_CASE and deep/copy mutable fields like
"exclude_types") and validate overrides: compute an allowed_keys set from
DPA3_BASELINE_CASE.keys() and raise KeyError for any key in overrides not in
allowed_keys before applying them, then apply overrides to the copied baseline
and proceed; reference DPA3_BASELINE_CASE, dpa3_case, and
dpa3_descriptor_api_case when making these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b00c1df4-7d0e-4a3d-8824-6aebf747064c
📒 Files selected for processing (1)
source/tests/consistent/descriptor/test_dpa3.py
…y unused unpacked variables in the curated DPA3\nconsistent tests so ruff no longer reports RUF059 on the helper\nproperties and setup code.\n\nAuthored by OpenClaw (model: gpt-5.4)
|
You're right — my previous reply was mismatched to an earlier review thread. I fixed the actual current comment now: the unused destructured case fields in — OpenClaw 2026.4.8 (model: gpt-5.4) |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
…eline case and validate override keys in the\ncurated-case helpers so mutable fields do not leak across cases and\ntypos fail fast.\n\nAuthored by OpenClaw (model: gpt-5.4)
2 participants
Problem
source/tests/consistent/descriptor/test_dpa3.pystill used a Cartesian-product parameter matrix, so applying the curated-case approach from test(consistent): replace Cartesian product with curated matrices for descriptor tests #5378 to DPA3 required the same cleanup.Change
Notes
git diff --checkas a quick sanity check here.Authored by OpenClaw (model: gpt-5.4)
Summary by CodeRabbit