Skip to content
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

Merged
merged 25 commits into from
Nov 17, 2023

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Nov 7, 2023

If the first line of the doctest has # doctest: +RUN_IN_PYODIDE, then we will run it in Pyodide once for each runtime available.

  • add a test
  • update changelog

Copy link
Member

@ryanking13 ryanking13 left a 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]
Copy link
Member

Choose a reason for hiding this comment

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

👍

tests/test_doctest.py Outdated Show resolved Hide resolved
@hoodmane
Copy link
Member Author

it seems like playwright is not happy

I tried using the selenium fixture with getfixture but it would require some deep pytest magic. But Joe Marshall added get_browser_pyodide which is very helpful here. I think the problem here is that get_browser_pyodide doesn't work correctly with Playwright but I'm not sure why. It also didn't shut down correctly but I think I've fixed that.

I think the correct solution is to refactor the fixtures a bit to get the selenium runtime from a separate @cached function so we can also retrieve them from code. Then get_browser_pyodide can use that and avoid the code duplication and that will hopefully resolve the playwright problem.

@ryanking13
Copy link
Member

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.

@hoodmane
Copy link
Member Author

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.

@hoodmane
Copy link
Member Author

@ryanking13 Could you review this again? I changed the approach quite a bit.

Copy link
Member

@ryanking13 ryanking13 left a 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?

pytest_pyodide/fixture.py Show resolved Hide resolved
@hoodmane hoodmane merged commit c932803 into pyodide:main Nov 17, 2023
33 checks passed
@hoodmane hoodmane deleted the doctest-in-pyodide branch November 17, 2023 19:31
hoodmane added a commit to hoodmane/pyodide that referenced this pull request Nov 17, 2023
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
hoodmane added a commit to pyodide/pyodide that referenced this pull request Nov 19, 2023
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
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