Skip to content

PYTHON-5138 Convert setup_tests.py to a cli #2154

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 44 commits into from
Feb 21, 2025

Conversation

blink1073
Copy link
Member

@blink1073 blink1073 requested a review from NoahStapp February 20, 2025 19:17
Copy link
Contributor

@NoahStapp NoahStapp left a comment

Choose a reason for hiding this comment

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

Give this man a medal. A few minor comments, but overall looks great!

test_func = FunctionCall(func="run tests", vars=test_vars)
tasks.append(EvgTask(name=name, tags=tags, commands=[bootstrap_func, test_func]))

Copy link
Contributor

Choose a reason for hiding this comment

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

Intentional whitespace?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

@@ -2,5 +2,5 @@
set -eu
HERE=$(dirname ${BASH_SOURCE:-$0})
. $HERE/env.sh
SUCCESS=false TEST_FLE_GCP_AUTO=1 bash $HERE/setup-tests.sh
./.evergreen/just.sh setup-test kms gcp-fail
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the use of ./.evergreen/just.sh instead of bash "${PROJECT_DIRECTORY}"/.evergreen/just.sh intentional here? It would be nice to use a consistent way of invoking our test cli across test suites.

Copy link
Member Author

Choose a reason for hiding this comment

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

These scripts are going away in subsequent PRs.

OCSP_TLS_SHOULD_SUCCEED="${OCSP_TLS_SHOULD_SUCCEED}" \
bash "${PROJECT_DIRECTORY}"/.evergreen/just.sh setup-test
bash "${PROJECT_DIRECTORY}"/.evergreen/just.sh test-eg
OCSP_TLS_SHOULD_SUCCEED="${OCSP_TLS_SHOULD_SUCCEED}" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OCSP_TLS_SHOULD_SUCCEED="${OCSP_TLS_SHOULD_SUCCEED}" \
OCSP_TLS_SHOULD_SUCCEED="${OCSP_TLS_SHOULD_SUCCEED}" \

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird, the UI won't let me accept the suggestion, will apply these manually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, this indent was intentional, and like I said, this script is going away.

bash "${PROJECT_DIRECTORY}"/.evergreen/just.sh setup-test
bash "${PROJECT_DIRECTORY}"/.evergreen/just.sh test-eg
OCSP_TLS_SHOULD_SUCCEED="${OCSP_TLS_SHOULD_SUCCEED}" \
bash scripts/setup-tests.sh ocsp
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bash scripts/setup-tests.sh ocsp
bash scripts/setup-tests.sh ocsp

pushd "$(dirname "$(dirname $HERE)")" > /dev/null
HERE="$( cd -- "$HERE" > /dev/null 2>&1 && pwd )"
ROOT=$(dirname "$(dirname $HERE)")
pushd $ROOT > /dev/null
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing matching popd?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@blink1073
Copy link
Member Author

I need to debug why a couple of the tests timed out.

@blink1073
Copy link
Member Author

Ah, we weren't properly handling NO_EXT before. I'll have to split those tests into sync and async.

@blink1073 blink1073 requested a review from NoahStapp February 21, 2025 18:01
@blink1073 blink1073 merged commit 25b2d77 into mongodb:master Feb 21, 2025
46 of 51 checks passed
@blink1073 blink1073 deleted the setup-test-cli branch February 21, 2025 20:27
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