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

Add tests #1

Closed
rth opened this issue Sep 5, 2022 · 6 comments
Closed

Add tests #1

rth opened this issue Sep 5, 2022 · 6 comments

Comments

@rth
Copy link
Contributor

rth commented Sep 5, 2022

Nice work on this!

I think API wise having an explicit patch method is indeed a reasonable compromise to issue of how to make requests available (related pyodide/micropip#9). Maybe slightly more flexible,

pyodide_http.patch('requests')  # also accepts lists a input.

so that it's easier to extend to other packages, without adding too many of those methods.

In the end, does this work for you for binary files (in the main thread)? I'm no longer sure what the situation is there pyodide/pyodide#3062

It would be nice to add some CI and tests e.g. using pytest-pyodide.

@koenvo
Copy link
Owner

koenvo commented Sep 5, 2022

Thanks for your kind works, @rth!

The reason for me to use the patch is that I always need some boiler-plate code to make it work. And in this case I made sure the patch doesn't break anything when it's not needed; in non-pyodide environments.

Regarding the binary files, I first had a different implementation (not in this repo) with xhr.responseText and that used to work for binary data. But somehow it stopped working. Current version (in this repo) uses a different approach and works with binary data.
To be honest, I'm not totally sure it's running in the main thread in my use-case. It's in JupyterLite.. You can find a working example here: https://playground.pysport.org/retro/notebooks/?path=mplsoccer_/plot_radar.ipynb (source for playground can be found at https://github.com/PySport/playground)

I like your proposal on the API. Let me change that.

Let me also look into pytest-pyodide. Thanks for the pointer!

@rth
Copy link
Contributor Author

rth commented Sep 5, 2022

Yeah, JupyterLite runs in a webworker, so in any case, so it should not be a problem there.

FYI in related projects there are https://github.com/emscripten-forge/requests-wasm-polyfill and work by @bartbroere in pyodide/pyodide#1956.

Once this package is a bit more used and tested in the future we could consider adding it to the pyodide Github org, if that's something that interests you.

Let me also look into pytest-pyodide

If you have any questions or things not working as expected please don't hesitate opening issues there. That package is fairly new.

@koenvo
Copy link
Owner

koenvo commented Sep 19, 2022

I’ve been trying to get the test dependencies working. Is it supported to work on macOS with python3.10? When I try to start it it says something about missing node-fetch package.

Is there somewhere a fully working example I can copy and adjust?

@rth
Copy link
Contributor Author

rth commented Sep 19, 2022

If you want to run tests in node, see https://www.npmjs.com/package/pyodide section "Node.js versions <0.17", yes, you would need to install node-fetch additionally, depending on your node version. Locally running tests in node is indeed the easiest (as compared to selenium with a browser)

I recently added tests in pyodide/micropip#3 you can adapt .github/workflows/main.yml from that repo (all the changes in tests/vendored are specific to that repo, so you can ignore them, otherwise the diff is rather short). This only runs tests on Chrome with selenium which is a start, and there is a more complete test setup in https://github.com/pyodide/pytest-pyodide/blob/main/.github/workflows/main.yml#L56 which tests various browsers.

If anything is unclear (including the docs), please feel free to complain at https://github.com/pyodide/pytest-pyodide as I believe you are one of the first users of this package :)

FIY, there is some interest in making requests work in jjjake/internetarchive#550 (waiting for their response, following a discussion on Twitter). If you have anything to add don't hesitate to comment there. If they go that road, they might need to improve this package as well, since they use a lot of the requests API, some of which currently still doesn't work with http-requests, last time I tried.

@koenvo
Copy link
Owner

koenvo commented Sep 19, 2022

Thank you @rth! I'll check your example somewhere this week.

@koenvo
Copy link
Owner

koenvo commented Oct 8, 2022

Fixed in #12 !

Thanks again to @joemarshall !

@koenvo koenvo closed this as completed Oct 8, 2022
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

No branches or pull requests

2 participants