Skip to content

refactor(client): pin 1.7.0 shape with deterministic structural assertions#342

Merged
imbajin merged 9 commits into
apache:mainfrom
Muawiya-contact:fix/backend-metrics-strict-assertion
Jun 8, 2026
Merged

refactor(client): pin 1.7.0 shape with deterministic structural assertions#342
imbajin merged 9 commits into
apache:mainfrom
Muawiya-contact:fix/backend-metrics-strict-assertion

Conversation

@Muawiya-contact

Copy link
Copy Markdown
Contributor

test(client): pin 1.7.0 backend_metrics shape with deterministic structural assertions

Closes #326


What does this PR do?

Replaces the loose assertGreater(len(...), 1) check in test_metric.py with deterministic structural assertions based on the actual /metrics/backend response captured from a real HugeGraph server.

The problem

The old assertion only checked length — shape drift in the API response would pass silently without any test failure.

# Before — too loose
self.assertGreater(len(backend_metrics["hugegraph"]), 1)

The fix

# After — deterministic structural assertion
missing_keys = EXPECTED_BACKEND_SERVER_KEYS - set(server_entry.keys())
self.assertFalse(missing_keys, f"Missing expected keys: {missing_keys}")

What was verified

  • Captured the actual /metrics/backend response from a real HugeGraph server
  • Pinned EXPECTED_BACKEND_SERVER_KEYS from that real response
  • Asserts backend, nodes, cluster_id, servers fields exist with correct types
  • Asserts all expected rocksdb metric keys are present in the server entry

Test results

Server version Result
HugeGraph 1.7.0 (Docker) ✅ PASSED
HugeGraph latest (Docker) ✅ PASSED

Both versions return identical shapes — one assertion covers both.

Files changed

Only one file touched: hugegraph-python-client/src/tests/api/test_metric.py

@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label May 22, 2026
@Muawiya-contact

Copy link
Copy Markdown
Contributor Author

@imbajin, any feedback please?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens the python-client integration test for GET /metrics/backend by replacing a loose length-only assertion with deterministic structural assertions that pin the expected HugeGraph 1.7.0 backend_metrics response shape (and intended to remain compatible with newer versions).

Changes:

  • Introduces a pinned set of expected metric keys (EXPECTED_BACKEND_SERVER_KEYS) for each backend server entry.
  • Replaces the old conditional/len-based assertion with explicit structural checks for graph entry fields (backend, nodes, cluster_id, servers) and expected RocksDB metrics.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread hugegraph-python-client/src/tests/api/test_metric.py Outdated
Comment thread hugegraph-python-client/src/tests/api/test_metric.py Outdated
Comment thread hugegraph-python-client/src/tests/api/test_metric.py
Comment thread hugegraph-python-client/src/tests/api/test_metric.py Outdated
Comment thread hugegraph-python-client/src/tests/api/test_metric.py Outdated
Comment thread hugegraph-python-client/src/tests/api/test_metric.py
- Replace strict single-graph check with assertGreaterEqual(>=1)
  to support multi-graph environments
- Add assertIsInstance for cluster_id to enforce documented str type
- Loop over all servers dict entries instead of checking only the first,
  ensuring every server entry is fully validated against expected RocksDB
  metric keys

No functional change; test coverage and portability improved.
Verified: ruff check + ruff format clean. Live backend test pending CI.

Signed-off-by: Muawiya-contact <contactmuawia@gmail.com>
Signed-off-by: Muawiya-contact <contactmuawia@gmail.com>
@Muawiya-contact

Copy link
Copy Markdown
Contributor Author

Hey @imbajin — all 13 CI checks are green and Copilot's review looks good too. Would love to get your eyes on this when you have a moment. Thanks!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

Comment thread hugegraph-python-client/src/tests/api/test_metric.py Outdated
Comment thread hugegraph-python-client/src/tests/api/test_metric.py Outdated
Comment thread hugegraph-python-client/src/tests/api/test_metric.py Outdated
@Muawiya-contact

Copy link
Copy Markdown
Contributor Author

@imbajin, Done ✅

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread hugegraph-python-client/src/tests/api/test_metric.py
@Muawiya-contact

Copy link
Copy Markdown
Contributor Author

@imbajin I have added now...

@imbajin

imbajin commented Jun 2, 2026

Copy link
Copy Markdown
Member

Note we refact the test frame in #357

@Muawiya-contact

Copy link
Copy Markdown
Contributor Author

Thanks for the heads-up @imbajin.

I checked #357 and understand the test framework has been refactored there. My intention in #342 was specifically to restore deterministic structural assertions for backend_metrics.

If there are any adjustments needed to align this PR with the new test framework in #357, I'd be happy to update it. Thanks for reviewing.

@imbajin

imbajin commented Jun 2, 2026

Copy link
Copy Markdown
Member

just resolve the conflicts 🔢

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread hugegraph-python-client/src/tests/api/test_metric.py

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One remaining portability issue: the test now pins RocksDB-specific backend metric fields in a python-client integration test. The graph-key concern is already covered by existing Copilot comments.

Comment thread hugegraph-python-client/src/tests/api/test_metric.py Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@Muawiya-contact Muawiya-contact requested a review from imbajin June 7, 2026 11:14

@imbajin imbajin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, LGTM. The current head addresses the previous graph-key selection and RocksDB portability concerns, and the client integration jobs pass against the HugeGraph 1.7.0 baseline.

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 8, 2026
@imbajin imbajin changed the title test(client): pin 1.7.0 backend_metrics shape with deterministic structural assertions refactor(client): pin 1.7.0 shape with deterministic structural assertions Jun 8, 2026
@imbajin imbajin merged commit 6272a22 into apache:main Jun 8, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer python-client size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] test: restore strict assertion for backend_metrics in test_metric.py

3 participants