Skip to content

PYTHON-3958 BSON failure - TestDatetimeConversion.test_millis_from_da… #1394

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
Oct 19, 2023

Conversation

NoahStapp
Copy link
Contributor

Resolve by always compiling the C extensions as part of tox -e test, adding about 15-20 seconds to each test run. Is this an acceptable tradeoff for ensuring the C extensions are always up to date?

@NoahStapp NoahStapp requested a review from a team as a code owner October 16, 2023 20:32
@NoahStapp NoahStapp requested review from Jibola and removed request for a team October 16, 2023 20:32
@ShaneHarvey
Copy link
Member

adding about 15-20 seconds to each test run

Is it every test run or only the first time?

@NoahStapp
Copy link
Contributor Author

adding about 15-20 seconds to each test run

Is it every test run or only the first time?

It's part of tox -e test, so only when that task is run. The various EG tests do not use tox -e test, so their runtime should be unaffected.

@blink1073
Copy link
Member

What if we had a script that builds the c ext if the built files don't exist? We could also consider comparing modified times to mimic make

@NoahStapp
Copy link
Contributor Author

What if we had a script that builds the c ext if the built files don't exist? We could also consider comparing modified times to mimic make

I like these ideas. However, then you can have the situation where you have old C extension files that prevent the rebuild from taking place. Building them on every tox -e test run avoids this issue.

@ShaneHarvey
Copy link
Member

Oh I think this is fine then. Personally I don't even use tox -e test at all locally because it's already so slow to setup. Instead I run pytest directly.

@Jibola
Copy link
Contributor

Jibola commented Oct 17, 2023

Resolve by always compiling the C extensions as part of tox -e test, adding about 15-20 seconds to each test run. Is this an acceptable tradeoff for ensuring the C extensions are always up to date?

I think it's fine. Do we want to make another step in tox that only compiles the C binary? Or potentially just supply a generalized flag like --rebuild-c-ext that we can supply to every test to run the recompilation on test set up.

Not sure how much of additional scope creep that becomes, but having that as a pre-cursor step would empower autonomy regardless of workflow.

@NoahStapp
Copy link
Contributor Author

Resolve by always compiling the C extensions as part of tox -e test, adding about 15-20 seconds to each test run. Is this an acceptable tradeoff for ensuring the C extensions are always up to date?

I think it's fine. Do we want to make another step in tox that only compiles the C binary? Or potentially just supply a generalized flag like --rebuild-c-ext that we can supply to every test to run the recompilation on test set up.

Not sure how much of additional scope creep that becomes, but having that as a pre-cursor step would empower autonomy regardless of workflow.

We could make another step for compiling the C extensions, that also gives us a slight amount of abstraction for when we fully migrate away from setuptools.

@NoahStapp NoahStapp merged commit 61269c0 into mongodb:master Oct 19, 2023
@NoahStapp NoahStapp deleted the PYTHON-3958 branch October 19, 2023 18:46
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.

4 participants