-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
ENH Add support for running doctests in pyodide #117
Conversation
If the first line of the doctest has # doctest: +RUN_IN_PYODIDE, then we will run it in Pyodide once for each runtime available.
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.
Thanks @hoodmane! Codewise LGTM, but it seems like playwright is not happy for some reason.
if not hasattr(pytest, "pyodide_options_stack"): | ||
pytest.pyodide_options_stack = [] | ||
else: | ||
pytest.pyodide_options_stack.append( # type:ignore[attr-defined] |
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 tried using the selenium fixture with I think the correct solution is to refactor the fixtures a bit to get the selenium runtime from a separate |
Thanks for the refactorings! About the test failure on safari, there is a known issue that safari webdriver does not allow opening multiple sessions, and we are handling it in a very ad-hoc way. So I am personally okay with disabling the doctest for safari if you think it is hard to handle. |
Well this PR has been a lot more work than I expected. I'll give it another try and see if I can get it to work. One thing worth pointing out is that the reason this is hard is because of support for multiple runtimes. If we required only one runtime per run, there wouldn't be any difficulty at all. |
for more information, see https://pre-commit.ci
…de into doctest-in-pyodide
@ryanking13 Could you review this again? I changed the approach quite a bit. |
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.
Thanks @hoodmane!
One thing worth pointing out is that the reason this is hard is because of support for multiple runtimes. If we required only one runtime per run, there wouldn't be any difficulty at all.
If this make things too complicated, I think it's fine to only allow one runtime to be set up per run; at least within the Pyodide organization, we don't use multiple runtimes at the same time.
@joemarshall What do you think? If we go in this way, this would be a breaking change, will this cause some problem to you?
This uses pyodide/pytest-pyodide#117 to run doctests in Pyodide. I also turned on and fixed various doctests that were not working for unrelated reasons
This uses pyodide/pytest-pyodide#117 to run doctests in Pyodide. I also turned on and fixed various doctests that were not working for unrelated reasons
If the first line of the doctest has
# doctest: +RUN_IN_PYODIDE
, then we will run it in Pyodide once for each runtime available.