Skip to content

Add string operator $replaceAll/$replaceOne tests#79

Merged
nitinahuja89 merged 5 commits intodocumentdb:mainfrom
eerxuan:upstream-replace-tests
Apr 17, 2026
Merged

Add string operator $replaceAll/$replaceOne tests#79
nitinahuja89 merged 5 commits intodocumentdb:mainfrom
eerxuan:upstream-replace-tests

Conversation

@eerxuan
Copy link
Copy Markdown
Collaborator

@eerxuan eerxuan commented Apr 9, 2026

Add compatibility tests for the $replaceAll and $replaceOne operators.

Ref: #10

Tests cover:

  • Core replacement behavior
  • Encoding and special characters
  • Type errors and invalid arguments
  • Null/missing handling
  • Size limits
  • Invariants and type precedence
  • Whitespace handling
  • Expression arguments and document field usage

497 tests total (3 known failures: empty_find_multibyte tests fail due to pymongo UTF-8 decoding of invalid byte sequences).

@eerxuan eerxuan requested a review from a team as a code owner April 9, 2026 23:52
@eerxuan eerxuan force-pushed the upstream-replace-tests branch from 622c724 to 0c969fb Compare April 9, 2026 23:55
Copy link
Copy Markdown
Collaborator

@danielfrankcom danielfrankcom left a comment

Choose a reason for hiding this comment

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

Seems like we might need to add some kind of pytest mark to indicate these tests are expected to fail for MongoDB as previously discussed? If we override the merge conditions here we'll end up with all future PRs failing these checks which seems undesirable.

Copy link
Copy Markdown
Collaborator

@danielfrankcom danielfrankcom left a comment

Choose a reason for hiding this comment

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

Won't the use of xfail mean that the compatibility tests expect all compatible databases to fail this test?

Since in this case it may be a database-specific issue, we may need a different approach here or we may cause this to fail for other compatible implementations even though the same problem doesn't exist.

@danielfrankcom
Copy link
Copy Markdown
Collaborator

Won't the use of xfail mean that the compatibility tests expect all compatible databases to fail this test?

Since in this case it may be a database-specific issue, we may need a different approach here or we may cause this to fail for other compatible implementations even though the same problem doesn't exist.

I'm not sure whether this would work for us in practice, but it looks like the xfail docs show a condition parameter. We may be able to isolate the xfail behavior only to MongoDB.

@danielfrankcom
Copy link
Copy Markdown
Collaborator

Seems like the condition needs to evaluate at setup time so accessing the engine name would be tricky.

Haven't tested this, but we might need to do something like this:

def pytest_configure(config):
    # We might be able to do this in the static `.ini` file instead, not sure.
    config.addinivalue_line(
        "markers",
        "engine_xfail(engine, reason): expected failure for a specific engine",
    )


def pytest_runtest_setup(item):
    for marker in item.iter_markers("engine_xfail"):
        if item.config.engine_name == marker.kwargs.get("engine"):
            pytest.xfail(marker.kwargs.get("reason", ""))

And then in the test file:

marks=(pytest.mark.engine_xfail(engine="mongodb", reason="MongoDB returns invalid UTF-8 for empty-find multibyte"),),

Comment thread documentdb_tests/conftest.py Outdated
@eerxuan eerxuan force-pushed the upstream-replace-tests branch 3 times, most recently from df85c3d to 6fff15a Compare April 14, 2026 01:13
Copy link
Copy Markdown
Collaborator

@danielfrankcom danielfrankcom left a comment

Choose a reason for hiding this comment

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

Most comments are around the introduction of xfail, which I guess is the downside of folding this change into an existing PR with test additions.

Comment thread documentdb_tests/framework/assertions.py
Comment thread documentdb_tests/conftest.py Outdated
@eerxuan eerxuan force-pushed the upstream-replace-tests branch from 6fff15a to eaf0342 Compare April 14, 2026 19:44
@eerxuan eerxuan force-pushed the upstream-replace-tests branch from 1ed612f to eaf0342 Compare April 16, 2026 19:39
Comment thread documentdb_tests/framework/assertions.py Outdated
@eerxuan eerxuan force-pushed the upstream-replace-tests branch 2 times, most recently from 90093a1 to a042cd8 Compare April 16, 2026 20:21
@eerxuan eerxuan force-pushed the upstream-replace-tests branch from a042cd8 to 946780e Compare April 16, 2026 20:23
danielfrankcom and others added 4 commits April 17, 2026 13:52
Signed-off-by: Daniel Frankcom <frankcom@amazon.com>
- Add __init__.py for package resolution
- Add ExpressionTestCase to utils/
- Add REPLACE_* error codes, FAILED_TO_PARSE_ERROR, INVALID_DOLLAR_FIELD_PATH, STRING_SIZE_LIMIT_ERROR to error_codes.py
- Add STRING_SIZE_LIMIT_BYTES to test_constants.py
- Fix pytest_params import (parametrize module)
- Use relative imports for operator common utils
- Pin CI MongoDB to 8.2.4
- Run isort/black formatting

Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
- Add _truncate_repr() to cap custom error messages at 1000 chars
- Use raise AssertionError for large values to suppress pytest introspection
- Keep bare assert for normal-sized values to preserve pytest diff
- Add truncation_limit_lines/chars to pytest.ini as safety net

Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>

# Conflicts:
#	documentdb_tests/framework/assertions.py
- Fix replaceOne/replaceAll tests to use assert_expression_result
- Add unit marker to pytest.ini

Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
Signed-off-by: Yunxuan Shi <yunxuan@amazon.com>
@nitinahuja89 nitinahuja89 force-pushed the upstream-replace-tests branch from 58bea3f to 32e67f5 Compare April 17, 2026 20:52
@nitinahuja89 nitinahuja89 merged commit 094b594 into documentdb:main Apr 17, 2026
5 checks passed
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.

3 participants