Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Run python tests in Pyodide build #11914
Run python tests in Pyodide build #11914
Changes from all commits
6340d08
2b8a86a
438e9d0
e386ebd
a5acba8
15472d1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not entirely confident this will be unique for every test
I have more faith in this fixture, do you mind using this?
tmp_path_factory creates unique paths, and numbers them, + for good measure I included the name of the test as another unique token
We are creating more than one files in certain places, perhaps this fixture should return a generator then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pytest documentation for
tmp_path
says almost verbatim exactly that, which is why I chose it :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation of
tmp_path
is very similar to your proposed implementation (I removed the docstring and clean up code that follows theyield
):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message is a bit coarse-grained here:
What fails is:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on this?
It seems to be working on https://duckdb.github.io/duckdb-pyodide/console by doing:
unsure if the problem is in the testing (possibly due to handling of timezones / else) or this is actually broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, try the code from the test this is marking.
Regardless, I was hoping this could be fixed in a follow-up, as this bug existed before we were running the test suite, so adding the tests is not making anything worse 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@carlopi Here's an example that fails:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error I get is: