Skip to content

Add test format validation, error codes framework, and $divide tests#22

Open
eerxuan wants to merge 2 commits intodocumentdb:mainfrom
eerxuan:format-validation-divide-tests-2
Open

Add test format validation, error codes framework, and $divide tests#22
eerxuan wants to merge 2 commits intodocumentdb:mainfrom
eerxuan:format-validation-divide-tests-2

Conversation

@eerxuan
Copy link
Copy Markdown
Contributor

@eerxuan eerxuan commented Mar 31, 2026

Adding test format auto validation in pytest test pickup hook.
Adding error code invariants.
Adding $divide test as a sample to show case the test format.

@eerxuan eerxuan requested a review from a team as a code owner March 31, 2026 20:00
@eerxuan eerxuan force-pushed the format-validation-divide-tests-2 branch from cad86fe to 54c1a08 Compare March 31, 2026 20:07
@eerxuan
Copy link
Copy Markdown
Contributor Author

eerxuan commented Mar 31, 2026

The [PR Tests / Tests (MongoDB) (pull_request)] will pass after the previous PR is merged. #20
Will rebase after.

Copy link
Copy Markdown

@xgerman xgerman left a comment

Choose a reason for hiding this comment

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

AI:
Issues Found

🔴 High — Duplicate imports in utils/init.py

from dataclasses import dataclass # duplicated
from typing import Any, Optional # duplicated

Lines 1-2 and 4-5 are identical. Copy-paste error.

🔴 High — Wildcard imports in utils/init.py

from documentdb_tests.framework.test_constants import *
from documentdb_tests.framework.error_codes import *

Wildcard imports pollute the namespace and make it unclear what's available. Should use explicit imports. This is
especially problematic since test_constants.py exports 80+ symbols.

🟡 Medium — test_structure_validator.py validation is now split but redundant The old check
test_{parent_folder}_*.py is now split into two separate checks:

  1. File starts with test_
  2. File name contains parent folder name

But this means test_smoke_expression_divide.py in folder divide/ passes check 2 because it contains "divide".
However test_operator_divide.py also passes. The original stricter pattern (test_divide_*.py) was more
prescriptive. The new looser check could allow my_test_that_mentions_divide_once.py. Not a blocker but worth
noting.

🟡 Medium — CONTRIBUTING.md has syntax error in example

execute_command(collection, {"find": collection.name, filter: {"a": 1}})

filter should be "filter" (string key). This will cause a NameError in Python.

🟡 Medium — test_format_validator.py missing newline at EOF Minor but will cause linter warnings.

🟡 Medium — Smoke test expected values may be wrong

expected = [{"_id": 1, "quotient": 5}, {"_id": 2, "quotient": 6}]

$divide returns doubles: 20/4 = 5.0 not 5, 30/5 = 6.0 not 6. The assertion may fail depending on how
assertSuccess compares types.

🟢 Minor — TEST_COVERAGE.md is 355 lines — very thorough but dense Great as a reference doc but could overwhelm
new contributors. Consider adding a "start here" callout at the top pointing to TEST_FORMAT.md and the $divide
example.

🟢 Minor — MISSING = "$missing" sentinel Using the string "$missing" as a sentinel works but is fragile — if any
test legitimately needs to test the literal string "$missing", it would collide. A _MISSING = object() sentinel
would be safer.

🟢 Minor — DOUBLE_PRECISION_LOSS = 9007199254740993 This is 2^53 + 1, which can't be exactly represented as a
float64. The Python literal 9007199254740993 is an int, but when converted to float it becomes 9007199254740992.0
. This is intentional (testing precision loss) but worth a comment.

Documentation Quality — Compared to pgcosmos/AGENTS.md

The pgcosmos AGENTS.md is excellent at providing actionable task aliases (build, test, start commands) and
institutional knowledge (which gateway is production, config gotchas). The functional-tests docs are strong on
test methodology but could borrow:

  • A "Quick Start" section — how to run the test suite in one command
  • Task aliases table — like pgcosmos does ("run tests" → pytest ..., "add a new operator test" → step-by-step)
  • Known limitations section — what doesn't work yet, known flaky tests

long → long (for operators that preserve integers)
decimal128 → decimal128
```
Note: Check each operator's specific return type rules. The general pattern is: decimal128 input always returns decimal128; other numeric types return double unless the operator preserves integer types.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bold note

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will remove the assumptions

Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
@eerxuan eerxuan force-pushed the format-validation-divide-tests-2 branch from 09cf53e to 0094b7d Compare April 2, 2026 00:17
Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
@eerxuan
Copy link
Copy Markdown
Contributor Author

eerxuan commented Apr 2, 2026

🟡 Medium — Smoke test expected values may be wrong

expected = [{"_id": 1, "quotient": 5}, {"_id": 2, "quotient": 6}]

$divide returns doubles: 20/4 = 5.0 not 5, 30/5 = 6.0 not 6. The assertion may fail depending on how
assertSuccess compares types.

In compatibility test we won't fail on trailing zeros, as it will be flaky and not top priority to catch, we will have separate test cases to verify return type with $type in command. They will be add to the $divide test later to have full coverage.

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.

2 participants