Skip to content

Remove $documents dependency from expression test helpers - #715

Open
eerxuan wants to merge 5 commits into
documentdb:mainfrom
eerxuan:remove-documents-dependency-upstream
Open

Remove $documents dependency from expression test helpers#715
eerxuan wants to merge 5 commits into
documentdb:mainfrom
eerxuan:remove-documents-dependency-upstream

Conversation

@eerxuan

@eerxuan eerxuan commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

What

Remove the dependency on the $documents aggregation stage from the shared
expression test helpers (execute_expression, execute_project) and two
inline call sites in test_expressions_combination_variables.py.

Why

These tests use a collectionless {aggregate: 1} pipeline with
{$documents: [{}]} purely as a scaffold to feed a single empty document
into $project. They do not test $documents itself — yet through the
shared helpers, ~205 expression test files inherit a hard dependency on
$documents support. Any engine that does not implement $documents cannot
run these tests at all, even though the behavior under test has nothing to do
with that stage.

Removing this dependency lets the expression compatibility suite run against
engines that don't yet support the $documents feature.

How

Swap the scaffold for collection.insert_one({}) plus an aggregate over the
named collection:

+    collection.insert_one({})
     return execute_command(
         collection,
         {
-            "aggregate": 1,
+            "aggregate": collection.name,
             "pipeline": [
-                {"$documents": [{}]},
                 {"$project": {"_id": 0, "result": expression}},
             ],
             "cursor": {},
         },
     )

This is logically identical: both feed exactly one empty document into
$project, so literal expressions and field references (which resolve to
missing against an empty document) behave the same. The collection fixture is
function-scoped, so each test still runs against a fresh single-document
collection.

Scope

  • expressions/utils/utils.py — the two shared helpers (execute_expression,
    execute_project) inherited by ~205 files.
  • test_expressions_combination_variables.py — two inline $documents sites
    (test_let_two_lets_same_projection, test_let_error_cross_let_variable_ref).
  • The _with_insert sibling helpers are unchanged (they already insert a
    populated document).

The expression compatibility tests use a collectionless
`{aggregate: 1}` + `$documents: [{}]` pipeline purely as a scaffold
to feed a single empty document into $project. They do not test
$documents itself, yet they inherit a hard dependency on $documents
support from the shared helpers.

Swap the scaffold for `collection.insert_one({})` plus an aggregate
over the named collection. This is logically identical: both feed
exactly one empty document to $project, so literal expressions and
field references (which resolve to missing against an empty doc)
behave the same. The `collection` fixture is function-scoped, so each
test still runs against a fresh single-document collection.

Updates the two shared helpers (execute_expression, execute_project)
that ~205 files inherit, plus two inline call sites in
test_expressions_combination_variables.py. The _with_insert sibling
helpers are unchanged.

Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
@eerxuan
eerxuan requested a review from a team as a code owner August 6, 2026 23:19
@documentdb-triage-tool

Copy link
Copy Markdown

🤖 Auto-triaged by documentdb-triage-tool.

Applied: compatibility test, enhancement
Project fields suggested: Component test-coverage · Priority P2 · Effort M · Status Needs Review
Confidence: 0.90 (mixed)

Reasoning

component from path globs (test-coverage); effort from diff stats (22+11 LOC, 2 files); LLM: Removes a hard $documents dependency from shared expression test helpers, unblocking ~205 test files from running against engines that don't implement $documents, with a logically equivalent insert-based scaffold.

If a label is wrong, remove it manually and ping @patty-chow so the rules can be tuned. The bot will not re-label items that already have component labels.

@documentdb-triage-tool documentdb-triage-tool Bot added compatibility test Compatibility test related enhancement New feature or request labels Aug 7, 2026
eerxuan added 2 commits August 6, 2026 18:22
The $documents-removal commit made execute_expression/execute_project
insert a document and aggregate over the whole collection. Two newly
synced system-variable tests relied on the old $documents:[{}] contract
and broke:

- test_root_empty_document: needs a truly field-less input so $$ROOT is
  {}, but an inserted doc always carries an auto _id. It now shapes its
  own pipeline ($replaceWith:{$literal:{}}) instead of the shared helper.
- test_now_identical_across_getmore_batches: pre-loads 300 docs, so the
  whole-collection helper emitted 300 rows. It now uses an inline
  pipeline with $limit:1 to collapse to the single expected row.

The shared helpers stay on plain insert_one({}) so the ~359 literal
expression call sites gain no $replaceWith/$limit dependency.

Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
The replica-set test job was hitting GitHub's 6h max execution limit.
Root cause: the suite creates and drops tens of thousands of collections
per run; WiredTiger keeps an open fd per table it has touched (closing idle
ones only very lazily) and, on the replica set, dropped collections become
drop-pending idents whose files linger until majority-committed and swept.
So the descriptor count tracks total collections created over the run, not
the number live at once, and mid-run mongod runs out of descriptors. That
surfaces as a WiredTiger WT_PANIC (EMFILE -> fassert) and crashes the
server; every remaining test then blocks on connection timeouts and the
job crawls for hours. Standalone is unaffected because it reclaims dropped
tables immediately and never approaches the ceiling.

Two changes:

1. Raise ulimits.nofile to 1048576 (the limit MongoDB's own packaging ships)
   on both mongod targets. The replica set peaks near 60k open files partway
   through the suite; 64000 was not enough headroom -- it still crashed at
   ~31k collections -- so this is set well above the peak.

2. Add a target-death watchdog and a per-test --timeout to the test job so
   that if a target ever does die mid-run, the job fails in about a minute
   instead of timing out every remaining test against a dead target for
   hours. The watchdog sanity-checks its probe first and steps aside if it
   cannot read a healthy baseline, so it cannot false-abort a healthy run.

Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
@eerxuan
eerxuan force-pushed the remove-documents-dependency-upstream branch from bec907b to 3681e1b Compare August 12, 2026 23:22
These tests write documents to advance the logical clock (the behavior
under test), then assert a variable/literal expression via execute_expression.
The old helper ran collectionless ($documents: [{}]) so those writes never
affected it. The new helper aggregates over collection.name, so every document
the test wrote becomes an output row -- the assertion expected [{result: X}]
but got one identical row per document.

Each affected assertion evaluates only $$CLUSTER_TIME and Python-computed
literals (never a document field), so the value is identical on any row. Add
{$limit: 1} to collapse the aggregation to the single-row evaluation the
$documents scaffold used to provide -- preserving exactly what each test
verifies without reintroducing the $documents dependency this PR removes.
This is the same shaping the PR already applied to
test_now_identical_across_getmore_batches. The 5 tests whose collection stays
empty are unaffected and still use the helper directly.

Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
Watchdog (pr-tests.yml): require two consecutive low container-count
readings before aborting. A single reading of the resource-heavy suite's
`docker compose ps` can transiently error and read as 0, which would
false-abort a healthy run; a real crash stays low across polls, so this
rules out the false abort while adding at most one poll interval to a
genuine fast-fail.

Helper docstrings (utils.py): the insert+aggregate form leaves the input
row with an auto-generated `_id` rather than the field-less row
`$documents: [{}]` produced, so note that an expression reading `$_id`,
`$$ROOT`, or `$$CURRENT` sees that id and diverges (references to any other
missing field are still identical). Documentation only; no behavior change.

Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

compatibility test Compatibility test related enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant