Skip to content

Update test suite #2181

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 31 commits into from
Sep 25, 2024
Merged

Update test suite #2181

merged 31 commits into from
Sep 25, 2024

Conversation

ntoll
Copy link
Member

@ntoll ntoll commented Sep 23, 2024

Description

This is a major refactor of the Python related testing of PyScript.

Detail - PyScript lives in two worlds: Python and JavaScript.

We have lots of JS tests and some clunky Python tests (originally run by playwright with the Python tests expressed as strings!). This refactor makes use of uPyTest (a PyTest inspired test framework that runs in PyScript in the browser) to run tests expressed as actual Python code in PyScript.

Changes

All the old Python integration tests have been deleted, with the following caveats:

  • Tests that used to exercise actual Python code inside PyScript have been copied over / refactored into the new Python integration tests.
  • Tests that check Pyodide or MicroPython have been removed (we should trust our upstream).
  • Tests that check PyScript is correctly running in the browser duplicate many of our existing JS tests (with JS tests being the more appropriate place to run these), have also been deleted.

I have also added tests for all the parts of the current Pythonic pyscript namespace available within PyScript. In addition to checking functionality, some of these tests are simple "sanity checks" to ensure features (such as pyscript.config or pyscript.RUNNING_IN_WORKER) are working.

Checklist

  • All tests pass locally
  • I have updated CHANGELOG.md
  • I have created documentation for this(if applicable)

pre-commit-ci bot and others added 20 commits September 23, 2024 16:54
…t step, since this is already built into the build step.
@@ -58,7 +58,7 @@
"@ungap/with-resolvers": "^0.1.0",
"@webreflection/idb-map": "^0.3.1",
"basic-devtools": "^0.1.6",
"polyscript": "^0.15.6",
"polyscript": "^0.15.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

Something went wrong with the previous MR ... this should be 0.15.8 instead! That being said, it's not clear to me why this file shows up with changes already merged in main ... I think you should rebase properly then change only this version of polyscript (or I can file a new MR).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm... this update clearly hasn't landed in main: https://github.com/pyscript/pyscript/blob/main/pyscript.core/package.json#L61 (I still see 0.15.7)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it doesn't mean it cannot land in here, it's the same code more or less, just one less PR to approve ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

this is still outdated ... can you please make it 0.15.8 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Dammit - I knew I'd missed something,

// terminal=0 to NOT have a terminal
const terminal = qs.get('terminal') !== '0';

// worker=1 to have a worker
Copy link
Contributor

Choose a reason for hiding this comment

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

I find these two a bit confusing ... we could as well just use has('terminal') or has('worker') ... not sure why we need =0 on terminal and =1 on worker ... this is a nit though, still maybe you agree.

Copy link
Member Author

Choose a reason for hiding this comment

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

This has more to do with my shonky JS "skillz". 😉 I'll fix this up... I agree with your proposed change. It's much more readable. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

it was probably me suggesting these two but that's because I forgot that ?terminal&worker are perfectly valid too as query strings parameters

<!DOCTYPE html>
<html lang="en">
<head>
<script src="mini-coi.js" scope="./"></script>
Copy link
Contributor

@WebReflection WebReflection Sep 24, 2024

Choose a reason for hiding this comment

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

as much as I think mini-coi validated in here could make some sense, I don't think we should test our code with a service worker ... our PyScript .com doesn't need or use one, so that test should likely be a part and very simple (if you can't change window.document.body.textContent from a worker, that's a failing Service Worker, nothing else literally changes).

I am saying this because behind the scene there is also sabayon SW approach but that's tested and code covered in sabayon already so we should provide "production code" (imho) ... your call (and mini-coi wasn't needed before)

@@ -0,0 +1,34 @@
# PyScript Test Suite

The test suite is a bit of a mess. We need to fix it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the purpose of this MR to fix this status? Just wondering if this file needs any update or if we should just remove (and I'll follow up with more cleanup around JS, if needed).

@ntoll
Copy link
Member Author

ntoll commented Sep 25, 2024

@WebReflection thanks for the feedback. Yes - this is very much still a WiP and I'll fix up things later today (currently about to step into a meeting).

I'll also move the javascript-integration to the right (new) place. We need to discuss where the other tests live. I assume some will go in "tests/manual" and others could be converted to JS integration tests.

@WebReflection
Copy link
Contributor

@ntoll

I assume some will go in "tests/manual" and others could be converted to JS integration tests.

integration tests that are in integration should remain integration tests ... manual tests should remain manual. Where we move these doesn't change their nature but also I don't think we need to do everything in this MR or it will delay for very little gain in terms of changes (because the old slow tests suit still runs on each commit).

@ntoll ntoll marked this pull request as ready for review September 25, 2024 13:02
@ntoll
Copy link
Member Author

ntoll commented Sep 25, 2024

@WebReflection OK. I've moved things around as we discussed so that:

  • tests/python - Python test suite.
  • tests/javascript - Formerly the js-integration tests.
  • tests/manual - remain the same, in that they contain "manual" tests.
  • TODO: All the other directories in tests/ should be moved to tests/manual or tests/javascript or deleted, depending on what they do.
  • I've also updated the READMEs, as needed.

HEADS UP: I've noticed that the MicroPython + workers JavaScript test sometimes times out. Somehow it appears to get stuck in some way.

@WebReflection
Copy link
Contributor

@ntoll

HEADS UP: I've noticed that the MicroPython + workers JavaScript test sometimes times out. Somehow it appears to get stuck in some way.

sometimes it's a CDN issue so it takes long time to even bootstrap the interpreter ... bear in mind that's not our own CDN where we serve PyScript, I am not sure how to tackle that but yes, it happened to me too and maybe we should investigate more if it's instead a matter of async checks thta are badly organized. If it keeps happening (very sporadic in my local env) it should get high priority.

@WebReflection
Copy link
Contributor

@ntoll actually ... I see you just increased the timeout ... I am not sure that's the right fix but if it makes tests less flaky I am OK with it, hence my approval.

@ntoll ntoll merged commit 06138bb into main Sep 25, 2024
2 checks passed
@ntoll ntoll deleted the update-test-suite branch September 25, 2024 15:24
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