Skip to content

PHPLIB-1063: Import writeConcernError in unified tests #1090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented May 31, 2023

Fix PHPLIB-1063

Import test file: https://github.com/mongodb/specifications/blob/master/source/command-logging-and-monitoring/tests/monitoring/writeConcernError.json

I checked that the test effectively fails when I modify any value in the test file.

@GromNaN GromNaN force-pushed the PHPLIB-1063 branch 2 times, most recently from f7779da to 31261a8 Compare May 31, 2023 18:49
@@ -0,0 +1,155 @@
{
"description": "writeConcernError",
"schemaVersion": "1.12",
Copy link
Member Author

Choose a reason for hiding this comment

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

The schema version is downgraded from 1.13 to 1.12: this file uses none of the new fields of 1.13: observeLogMessages, logComponent, logSeverityLevel, expectedLogMessagesForClient, expectLogMessages.

Copy link
Member

Choose a reason for hiding this comment

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

This should be reverted to 1.13 for now. We don't modify the JSON test files when syncing them to the specs repo. Some other drivers, like libmongoc, do that, but IMO it becomes problematic as we're then unable to differentiate driver-specific changes from upstream changes.

Correcting this would be better done upstream. That would entail the overhead of a DRIVERS ticket, but you could avoid spawning language tickets by selecting "Driver Changes: Not Needed" like I did in DRIVERS-2641.

@alcaeus and I were actually discussing this last week based on a comment I left in mongodb/specifications#1423 (comment) (another spec test using a higher schema version than required). He'd like to explore adding a CI step to the specs repo to catch this and I assume the implementation would also entail revising existing spec test files. He'll likely create a DRIVERS ticket to track that when he's back from PTO, so I think we can just wait on that (and avoid fixing this test file upstream for now).

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot revert the schemaVersion without updating MAX_SCHEMA_VERSION. Which is even worse if the new fields are not implemented in the test runner.

/* Note: This is necessary to support expectedError.errorResponse from 1.12;
* however, syntax from 1.9, 1.10, and 1.11 has not been implemented. */
public const MAX_SCHEMA_VERSION = '1.12';

Copy link
Member

Choose a reason for hiding this comment

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

Noted. I think it's best to get this addressed upstream first, then. You can follow the process I used in DRIVERS-2641 to avoid extra language tickets.

Copy link
Member Author

Choose a reason for hiding this comment

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

@GromNaN GromNaN requested a review from jmikola May 31, 2023 18:51
@GromNaN GromNaN marked this pull request as ready for review May 31, 2023 18:51
Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

LGTM with the commit message modified and change to schemaVersion reverted.

@@ -0,0 +1,155 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

When syncing spec tests, please include a reference to the specs commit. For this PR, it should be:

Synced with mongodb/specifications@18932c00f9e2f8be66b48c930cbbc428499db668

This makes it easier to jump to the specs repo when viewing PHPLIB's git history.

@GromNaN GromNaN merged commit 7e9df62 into mongodb:master Jun 13, 2023
@GromNaN GromNaN deleted the PHPLIB-1063 branch June 13, 2023 10:08
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