PYTHON-5253 Automated Spec Test Sync#2409
Conversation
sleepyStick
left a comment
There was a problem hiding this comment.
Added some comments that I think might help the reviewer :)
| cd $SPECS/source | ||
| make | ||
| cd - | ||
| if ! [ -n "${CI:-}" ] |
There was a problem hiding this comment.
CI is always defined in an EVG run and we only want to run these things in a local checkout.
.evergreen/scripts/create-pr.sh
Outdated
| #!/usr/bin/env bash | ||
|
|
||
| tools="../drivers-evergreen-tools" | ||
| git clone https://github.com/mongodb-labs/drivers-evergreen-tools.git $tools |
There was a problem hiding this comment.
cloning drivers evergreen tools because there are some scripts (mainly ./secrets-export.sh and get-access-token.sh that are there and i'd rather not copy it over)
There was a problem hiding this comment.
Don't we already have a script to clone this repo?
There was a problem hiding this comment.
we do? I didn't know that. Do you know what it's called?
There was a problem hiding this comment.
.evergreen/scripts/configure-env.sh
There was a problem hiding this comment.
Thanks for pointing it out! I just removed the clone here and found the existing path for it!
| and len(list(os.scandir(pathlib.Path(entry.path) / "tests"))) > 1 | ||
| } | ||
| test_set = {entry.name.replace("-", "_") for entry in os.scandir(directory) if entry.is_dir()} | ||
| known_mappings = { |
There was a problem hiding this comment.
I think there's a slight difference in how we name the directories vs how the spec names them sometimes? (Not 100% sure if these are correct though, so please let me know if these need to be changed!)
There was a problem hiding this comment.
Yes we often have different names than the spec repo, that's expected.
| if spec.name in ["asynchronous"]: | ||
| continue | ||
| process = subprocess.run( | ||
| ["bash", "./.evergreen/resync-specs.sh", spec.name], # noqa: S603, S607 |
There was a problem hiding this comment.
Note:
S603 `subprocess` call: check for execution of untrusted input
S607 Starting a process with a partial executable path
S603: I think spec.name is safe because they're only acquired from our directories within test
S607: I think bash is sufficient for executable (they want the whole path)
| errored[spec.name] = process.stderr | ||
|
|
||
| process = subprocess.run( | ||
| ["git diff --name-only | awk -F'/' '{print $2}' | sort | uniq"], # noqa: S607 |
There was a problem hiding this comment.
Note:
S607 Starting a process with a partial executable path
I tried to separate the string into ["git", "diff --name-only | awk -F'/' '{print $2}' | sort | uniq"] but that caused the command to fail? So I kept it as a whole string..
There was a problem hiding this comment.
Each space-separated word has to be a separate arg, keeping it as one here is probably more readable.
.evergreen/specs.patch
Outdated
| @@ -0,0 +1,1327 @@ | |||
| diff --git a/test/load_balancer/cursors.json b/test/load_balancer/cursors.json | |||
There was a problem hiding this comment.
This is a file of known spec test differences that we don't want to be committed. If any one of these is incorrect, or I'm missing some, lmk!
There was a problem hiding this comment.
We should add a section to CONTRIBUTING.md detailing the why and how of modifying this file. It should also contain the policy we follow summarized by Shane in the Slack thread.
There was a problem hiding this comment.
Good point. Just added a blurb, but lmk if the language is unclear or anything like that. (I feel like my technical writing skills are lacking sometimes)
NoahStapp
left a comment
There was a problem hiding this comment.
Great work! Some minor comments.
| ["bash", "./.evergreen/resync-specs.sh", spec.name], # noqa: S603, S607 | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, |
There was a problem hiding this comment.
What's the motivation behind check=False here and then checking the returncode explicitly instead of using check=True?
There was a problem hiding this comment.
err no motivation (I learned about check when ruff yelled at me for it so) Changed it to check=True inside of a try catch!
| errored[spec.name] = process.stderr | ||
|
|
||
| process = subprocess.run( | ||
| ["git diff --name-only | awk -F'/' '{print $2}' | sort | uniq"], # noqa: S607 |
There was a problem hiding this comment.
Each space-separated word has to be a separate arg, keeping it as one here is probably more readable.
| and len(list(os.scandir(pathlib.Path(entry.path) / "tests"))) > 1 | ||
| } | ||
| test_set = {entry.name.replace("-", "_") for entry in os.scandir(directory) if entry.is_dir()} | ||
| known_mappings = { |
There was a problem hiding this comment.
Yes we often have different names than the spec repo, that's expected.
.evergreen/specs.patch
Outdated
| @@ -0,0 +1,1327 @@ | |||
| diff --git a/test/load_balancer/cursors.json b/test/load_balancer/cursors.json | |||
There was a problem hiding this comment.
We should add a section to CONTRIBUTING.md detailing the why and how of modifying this file. It should also contain the policy we follow summarized by Shane in the Slack thread.
|
I opened https://jira.mongodb.org/browse/PYTHON-5439 for the link failure |
NoahStapp
left a comment
There was a problem hiding this comment.
Great work! Added several suggestions for the CONTRIBUTING.md documentation section to ensure active voice and overall consistency.
| #!/bin/bash | ||
| PYMONGO=$(dirname "$(cd "$(dirname "$0")" || exit; pwd)") | ||
|
|
||
| rm -rf $PYMONGO/test/server_selection/logging |
There was a problem hiding this comment.
This should have a comment explaining why we remove this directory.
There was a problem hiding this comment.
I took this line from resync-spec.sh (moved it here for consistency) but there was no comment in resync-spec.sh for why its removed..do you happen to know why?
There was a problem hiding this comment.
We put those tests in test/server_selection_logging instead.
| token=$(bash ./get-access-token.sh $repo $owner) | ||
| if [ -z "${token}" ]; then | ||
| echo "Failed to get github access token!" | ||
| popd || exit |
There was a problem hiding this comment.
If we're going to exit anyway, why popd first?
There was a problem hiding this comment.
Good point....turns out i had originally just done a popd and then exit 1, but then my pycharm linter was like "yo, what if popd fails? you should add an || exit to be safe" so i did LOL but its kinda unnecessary cuz I know I did a pushd at the beginning of the script.
anyways, i removed the || exit
There was a problem hiding this comment.
wait jk, its not just pycharm linter, our liters say that too...(but it was fine when i ran it locally? 😕)
edit: i realized i didn't actually answer your question...i wanna popd before i exit so that the resync-all-specs.sh is at the path it expects to be on (i delete a file that the script generated to "cleanup" after calling this create-pr script)
Co-authored-by: Noah Stapp <noah@noahstapp.com>
Co-authored-by: Noah Stapp <noah@noahstapp.com>
Co-authored-by: Noah Stapp <noah@noahstapp.com>
Co-authored-by: Noah Stapp <noah@noahstapp.com>
Co-authored-by: Noah Stapp <noah@noahstapp.com>
Co-authored-by: Noah Stapp <noah@noahstapp.com>
Co-authored-by: Noah Stapp <noah@noahstapp.com>
Co-authored-by: Noah Stapp <noah@noahstapp.com>
NoahStapp
left a comment
There was a problem hiding this comment.
Comment needed for test/server_selection_logging.
Moved that line back to |
Generates at most one weekly PR on Monday's at 9am pst (assuming the evergreen hosts are in utc time...).
At a high level, I've written a script that simply loops through pymongo's existing spec tests and calls the existing
bash resync-spec.shto re-sync all the specs. Then if changes were made, a PR would be created. If all specs are up to date, then no PR will be made.This PR will be created by mongodb-drivers-pr-bot with the changes. If an error occurred during the syncing of a spec, the PR description will display the name of the spec along with stderr from the
bash resync-spec.sh <spec>command.An example of a generated PR #2407