Repository: pyodide/pyodide
Pull request: TST: Fix test_monkeypatch_eval_code
By restoring eval_code after the test is done. It's crazy that this bug didn't show up before!
Resolves #1200.

Problem statement:
TST Occasional failures of test_keyboard_interrupt
I have seen this test fail at least once in the CI and once locally. Rerunning the test fixes it, but it does suggest that it's maybe not very stable.
```
___________________________________________________________________________________ test_keyboard_interrupt[firefox] ____________________________________________________________________________________

selenium = <conftest.FirefoxWrapper object at 0x7f6963c4f7f0>

    def test_keyboard_interrupt(selenium):
>       assert selenium.run_js(
            """
            x = new Int8Array(1)
            pyodide._module.setInterruptBuffer(x)
            window.triggerKeyboardInterrupt = function(){
                x[0] = 2;
            }
            try {
                pyodide.runPython(`
                    from js import triggerKeyboardInterrupt
                    x = 0
                    while True:
                        x += 1
                        if x == 2000:
                            triggerKeyboardInterrupt()
                `)
            } catch(e){}
            return pyodide.runPython(`
                2000 < x < 2500
            `)
            """
        )
E       AssertionError: assert None
E        +  where None = <bound method SeleniumWrapper.run_js of <conftest.FirefoxWrapper object at 0x7f6963c4f7f0>>('\n        x = new Int8Array(1)\n        pyodide._module.setInterruptBuffer(x)\n        window.triggerKeyboardInterrup...       `)\n        } catch(e){}\n        return pyodide.runPython(`\n            2000 < x < 2500\n        `)\n        ')
E        +    where <bound method SeleniumWrapper.run_js of <conftest.FirefoxWrapper object at 0x7f6963c4f7f0>> = <conftest.FirefoxWrapper object at 0x7f6963c4f7f0>.run_js

src/tests/test_pyodide.py:198: AssertionError
```

On chrome it produces,
```
E       AssertionError: assert {}
E        +  where {} = <bound method SeleniumWrapper.run_js of <conftest.ChromeWrapper object at 0x7fa3564e7e80>>('\n        x = new Int8Array(1)\n        pyodide._module.setInterruptBuffer(x)\n        window.triggerKeyboardInterrup...       `)\n        } catch(e){}\n        return pyodide.runPython(`\n            2000 < x < 2500\n        `)\n        ')
E        +    where <bound method SeleniumWrapper.run_js of <conftest.ChromeWrapper object at 0x7fa3564e7e80>> = <conftest.ChromeWrapper object at 0x7fa3564e7e80>.run_js

src/tests/test_pyodide.py:198: AssertionError
--------------------------------------------------------------------------------------- Captured stdout teardown ----------------------------------------------------------------------------------------
Python exception:
Traceback (most recent call last):
  File "<exec>", line 6, in eval_code
  File "/lib/python3.8/site-packages/pyodide/_base.py", line 302, in eval_code
    return CodeRunner(
  File "/lib/python3.8/site-packages/pyodide/_base.py", line 225, in run
    exec(mod, self.globals, self.locals)
  File "<exec>", line 5, in <module>
KeyboardInterrupt
```

cc @hoodmane

Hints from issue discussion:
Actually I can consistently reproduce it with,
```
pytest src/tests/test_pyodide.py -k firefox
```
using both chrome or firefox. However when it's run separately,
```
pytest src/tests/test_pyodide.py -k "firefox and test_keyboard_interrupt"
```
it doesn't fail. So there is likely some bad interaction with other tests in `test_pyodide.py`. Which also explains why it occasionally fails in CI, as the run order is not fully deterministic with pytest-xdist.
Thanks for pointing this out @rth!

> likely some bad interaction with other tests

Weird, well that's great that it reproduces. Hopefully I'll get the same effect, I'll check it out and try to see if I can fix it.
This seems to have been caused by #1167 somehow.
In particular, it doesn't seem to be a problem with the test or the test suite, the type conversions logic is actually wrong now. Confusing...
Oh wait! It is a problem with the tests: `test_monkeypatch_eval_code` doesn't undo the monkey patch. It's actually weird that this hasn't shown up until now.

Commit message: Lint

Patch to review:
diff --git a/src/tests/test_pyodide.py b/src/tests/test_pyodide.py
index 22d6a972501..9b5e8b31e8d 100644
--- a/src/tests/test_pyodide.py
+++ b/src/tests/test_pyodide.py
@@ -146,6 +146,11 @@ def eval_code(code, ns):
     )
     assert selenium.run("x = 99; 5") == [3, 5]
     assert selenium.run("7") == [99, 7]
+    selenium.run(
+        """
+        pyodide.eval_code = old_eval_code
+        """
+    )
 
 
 def test_hiwire_is_promise(selenium):


Code review comments:
diff_hunk: @@ -146,6 +146,11 @@ def eval_code(code, ns):
     )
     assert selenium.run("x = 99; 5") == [3, 5]
     assert selenium.run("7") == [99, 7]
+    selenium.run(
+        """
+        pyodide.eval_code = old_eval_code
+        """
+    )
path: src/tests/test_pyodide.py
text: @user1:
Ideally we would want this in a finally block, so it's executed even if the test fails.

I think it didn't affect others tests too much because it's toward the end of the test suite, in the order it's usually run.

@author:
Right, good point. (We could also just remove this test entirely, I can't imagine why there would be a regression in this area.)