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

MAINT: Update conftest to check PyErr_Occurred #1113

Merged
merged 20 commits into from
Jan 13, 2021

Conversation

hoodmane
Copy link
Member

I think there are some bugs where pyodide leaves the python error flag in an inconsistent state. These bugs invoke unspecified behavior in CPython. They often go unnoticed because the error flag is frequently cleared or overwritten. However, under the right circumstances they can show up, often due to changes that don't directly touch the code causing the problem. For instance, bug #1110 was exposed in PR #1098 because I added extra error checking code, even though bug #1110 is caused by problems in python2js.c and python2js_buffer.c which were both unchanged in #1098.

I have seen similar symptoms to the ones caused by #1110 without hitting that particular codepath, so I strongly suspect there are still other similar bugs in the code. This will help by double checking whether the Python error indicator is in an invalid state at the end of each selenium.run_js call.

conftest.py Outdated Show resolved Hide resolved
conftest.py Outdated Show resolved Hide resolved
@hoodmane
Copy link
Member Author

@dalcde Every test fails here, the errors are TypeError: Cannot read property '_PyErr_Occurred' of undefined from pyodide._module._PyErr_Occurred. So the problem is pyodide._module is undefined. Why would that be?? Maybe if we changed pyodide.js like:

  let Module = {};
  Module._module = Module; // add this
  self.Module = Module;

@dalcde
Copy link
Contributor

dalcde commented Jan 11, 2021 via email

@hoodmane
Copy link
Member Author

@rth @dalcde The tests failed because now it correctly fails test_numpy.py::test_python2js_numpy_dtype which leaves the error indicator set.

@hoodmane hoodmane changed the title Update conftest to check PyErr_Occurred MAINT: Update conftest to check PyErr_Occurred Jan 11, 2021
@dalcde
Copy link
Contributor

dalcde commented Jan 12, 2021

I can merge this after the tests from #1111 are tidied up

@hoodmane
Copy link
Member Author

Okay, will do.

@hoodmane
Copy link
Member Author

Oops should have merged first.

@hoodmane
Copy link
Member Author

hoodmane commented Jan 12, 2021

Also, the array.array test uses Q as the format which means 64 bit integers. Javascript has a BigUint64Array. Ideally it would be good to make a memoryview with a more definitely invalid format to make the test more forward compatible.

@dalcde
Copy link
Contributor

dalcde commented Jan 12, 2021

For whatever inexplicable reason the tests seem to be genuinely failing

@hoodmane
Copy link
Member Author

I will try to figure out what's wrong tomorrow.

@hoodmane
Copy link
Member Author

hoodmane commented Jan 12, 2021

I also added a second test with the "u" format, which is the only alternative to "Q" for a format which triggers the bug and can be made with array.array

(numpy seems to make all sorts of other exotic format strings, from C I could make anything I want, but this seems good enough.)

@hoodmane
Copy link
Member Author

hoodmane commented Jan 12, 2021

Now the docs is failing because files.pythonhosted.org is returning a 502...

@dalcde dalcde merged commit 816aaa0 into pyodide:master Jan 13, 2021
@hoodmane hoodmane deleted the test-err-occurred branch January 13, 2021 10:08
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