8000 [EPIC] Fix all the remaining tests in the main thread · Issue #1761 · pyscript/pyscript · GitHub
[go: up one dir, main page]
Skip to content
8000

[EPIC] Fix all the remaining tests in the main thread #1761

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.

Sign up for GitHub

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

Jump to bottom
Closed
antocuni opened this issue Sep 27, 2023 · 1 comment
Closed

[EPIC] Fix all the remaining tests in the main thread #1761

antocuni opened this issue Sep 27, 2023 · 1 comment

Comments

@antocuni
Copy link
Contributor
antocuni commented Sep 27, 2023 •
edited
Loading

After we merge #1760, we have a number of tests which are skipped when running pyscript on the main thread.
I haven't investigated them in details yet, and I suspect/assume that some of them fail for the same/related reasons.
The goal of this issue is to give a rough explanation, try to group them into categories and serve as a starting point for further discussion, and to decide which ones we want to absolutely fix before the final release, and which ones can wait.

I expect that some of them will be uncontroversial, others will require more discussion, so depending on the case we might want to open separate sub-issues for them.

Missing error banner and/or error message

Note: some of these tests explicitly try to locate the .py-error banner. It's probably a good idea to rewrite them to use the self.assert_banner_message() method.

  • if the user specifies a package which doesn't exist, we should show a nice error in the DOM

    pyscript/pyscript.core/tests/integration/test_01_basic.py

    Lines 206 to 228 in abb4598

    @pytest.mark.skip("NEXT: No banner")
    def test_non_existent_package(self):
    self.pyscript_run(
    """
    <py-config>
    packages = ["i-dont-exist"]
    </py-config>
    <script type="py">
    print('hello')
    </script>
    """,
    wait_for_pyscript=False,
    )
    expected_alert_banner_msg = (
    "(PY1001): Unable to install package(s) 'i-dont-exist'. "
    "Unable to find package in PyPI. Please make sure you have "
    "entered a correct package name."
    )
    alert_banner = self.page.wait_for_selector(".alert-banner")
    assert expected_alert_banner_msg in alert_banner.inner_text()
    self.check_py_errors("Can't fetch metadata for 'i-dont-exist'")

  • this is similar to the above, but slightly different: in this case, the package exists but cannot be installed because it doesn't have a "pure python wheel". We should probably also write a relevant page in the docs and add a link to it in the error message

    pyscript/pyscript.core/tests/integration/test_01_basic.py

    Lines 230 to 251 in abb4598

    8000
    @pytest.mark.skip("NEXT: No banner")
    def test_no_python_wheel(self):
    self.pyscript_run(
    """
    <py-config>
    packages = ["opsdroid"]
    </py-config>
    <script type="py">
    print('hello')
    </script>
    """,
    wait_for_pyscript=False,
    )
    expected_alert_banner_msg = (
    "(PY1001): Unable to install package(s) 'opsdroid'. "
    "Reason: Can't find a pure Python 3 Wheel for package(s) 'opsdroid'"
    )
    alert_banner = self.page.wait_for_selector(".alert-banner")
    assert expected_alert_banner_msg in alert_banner.inner_text()
    self.check_py_errors("Can't find a pure Python 3 wheel for 'opsdroid'")

  • if the page contains two <py-config> tags, the 2nd is silently ignored. We should at least show a warning in that case:

    pyscript/pyscript.core/tests/integration/test_py_config.py

    Lines 106 to 130 in 03c09f1

    @pytest.mark.skip("NEXT: emit a warning in case of multiple py-config")
    def test_multiple_py_config(self):
    self.pyscript_run(
    """
    <py-config>
    name = "foobar"
    </py-config>
    <py-config>
    name = "this is ignored"
    </py-config>
    <script type="py">
    import js
    #config = js.pyscript_get_config()
    #js.console.log("config name:", config.name)
    </script>
    """
    )
    banner = self.page.wait_for_selector(".py-warning")
    expected = (
    "Multiple <py-config> tags detected. Only the first "
    "is going to be parsed, all the others will be ignored"
    )
    assert banner.text_content() == expected

  • if we try to [[fetch]] an URL which doesn't exist, we should show a nice error message in the DOM

    pyscript/pyscript.core/tests/integration/test_py_config.py

    Lines 155 to 175 in 03c09f1

    @pytest.mark.skip("NEXT: emit an error if fetch fails")
    def test_paths_that_do_not_exist(self):
    self.pyscript_run(
    """
    <py-config>
    [[fetch]]
    files = ["./f.py"]
    </py-config>
    <script type="py">
    print("this should not be printed")
    </script>
    """,
    wait_for_pyscript=False,
    )
    expected = "(PY0404): Fetching from URL ./f.py failed with " "error 404"
    inner_html = self.page.locator(".py-error").inner_html()
    assert expected in inner_html
    assert expected in self.console.error.lines[-1]
    assert self.console.log.lines == []

Accessing pyodide from JS API

PyScript classes exposed an API to access the underlying pyodide from JS: we introduced it because some people requested the funcionality, which was needed for some advanced use case. I have a vague remembering that @JeffersGlass was involved with it, so maybe he remembers more.

If I understand correctly, pyscript next offers another way of achieving the same result. If so, we should just fix/adapt these two tests to use the new functionality and document the difference:

pyscript/pyscript.core/tests/integration/test_01_basic.py

Lines 296 to 310 in 03c09f1

# TODO: ... and we shouldn't: it's a module and we better don't leak in global
@pytest.mark.skip("NEXT: we don't expose pyscript on window")
def test_js_version(self):
self.pyscript_run(
"""
<script type="py">
</script>
"""
)
self.page.add_script_tag(content="console.log(pyscript.version)")
assert (
re.match(r"\d{4}\.\d{2}\.\d+(\.[a-zA-Z0-9]+)?", self.console.log.lines[-1])
is not None
)

pyscript/pyscript.core/tests/integration/test_01_basic.py

Lines 312 to 335 in 03c09f1

# TODO: ... and we shouldn't: it's a module and we better don't leak in global
@pytest.mark.skip("NEXT: we don't expose pyscript on window")
def test_python_version(self):
self.pyscript_run(
"""
<script type="py">
import js
js.console.log(pyscript.__version__)
js.console.log(str(pyscript.version_info))
</script>
"""
)
assert (
re.match(r"\d{4}\.\d{2}\.\d+(\.[a-zA-Z0-9]+)?", self.console.log.lines[-2])
is not None
)
assert (
re.match(
r"version_info\(year=\d{4}, month=\d{2}, "
r"minor=\d+, releaselevel='([a-zA-Z0-9]+)?'\)",
self.console.log.lines[-1],
)
is not None
)

JS API difference between <script type="py"> and <py-script>:

I think we have already discussed this one, but I don't remember what was the outcome of the discussion :)

pyscript/pyscript.core/tests/integration/test_01_basic.py

Lines 337 to 355 in 03c09f1

@pytest.mark.skip("NEXT: works with <py-script> not with <script>")
def test_getPySrc_returns_source_code(self):
self.pyscript_run(
"""
<py-script>print("hello from py-script")</py-script>
<script type="py">print("hello from script py")</script>
"""
)
pyscript_tag = self.page.locator("py-script")
assert pyscript_tag.inner_html() == ""
assert (
pyscript_tag.evaluate("node => node.srcCode")
== 'print("hello from py-script")'
)
script_py_tag = self.page.locator('script[type="py"]')
assert (
script_py_tag.evaluate("node => node.srcCode")
== 'print("hello from script py")'
)

This is an entire test file which is completely skipped and seem to be related:

pyscript/pyscript.core/tests/integration/test_interpreter.py

Lines 5 to 12 in c6aaacd

pytest.skip(
reason="NEXT: pyscript API changed doesn't expose pyscript to window anymore",
allow_module_level=True,
)
class TestInterpreterAccess(PyScriptTest):
"""Test accessing Python objects from JS via pyscript.interpreter"""

Accessing config

Another thing which we have already discussed: I think that now we have an official way to get the config from python, we should rewrite the tests to use that

pyscript/pyscript.core/tests/integration/test_py_config.py

Lines 34 to 49 in 03c09f1

@pytest.mark.skip("NEXT: works with <py-script> not with <script>")
def test_py_config_inline_scriptpy(self):
self.pyscript_run(
"""
<py-config>
name = "foobar"
</py-config>
<script type="py" async>
from pyscript import window, document
promise = await document.currentScript._pyodide.promise
window.console.log("config name:", promise.config.name)
</script>
"""
)
assert self.console.log.lines[-1] == "config name: foobar"

pyscript/pyscript.core/tests/integration/test_py_config.py

Lines 51 to 68 in 03c09f1

@pytest.mark.skip("NEXT: works with <py-script> not with <script>")
def test_py_config_external(self):
pyconfig_toml = """
name = "app with external config"
"""
self.writefile("pyconfig.toml", pyconfig_toml)
self.pyscript_run(
"""
<py-config src="pyconfig.toml"></py-config>
<script type="py" async>
from pyscript import window, document
promise = await document.currentScript._pyodide.promise
window.console.log("config name:", promise.config.name)
</script>
"""
)
assert self.console.log.lines[-1] == "config name: app with external config"

output and stderr attributes

These tests fail because output and stderr are no longer supported. Do we want to fix it, or we have decided to remove the functionality?

pyscript/pyscript.core/tests/integration/test_script_type.py

Lines 97 to 108 in 03c09f1

@pytest.mark.skip("FIXME: output attribute is not implemented")
def test_script_type_py_output_attribute(self):
self.pyscript_run(
"""
<div id="first"></div>
<script type="py" output="first">
print("<p>Hello</p>")
</script>
"""
)
text = self.page.locator("#first").text_content()
assert "<p>Hello</p>" in text

pyscript/pyscript.core/tests/integration/test_script_type.py

Lines 110 to 124 in 03c09f1

@pytest.mark.skip("FIXME: stderr attribute is not implemented")
def test_script_type_py_stderr_attribute(self):
self.pyscript_run(
"""
<div id="stdout-div"></div>
<div id="stderr-div"></div>
<script type="py" output="stdout-div" stderr="stderr-div">
import sys
print("one.", file=sys.stderr)
print("two.")
</script>
"""
)
assert self.page.locator("#stdout-div").text_content() == "one.two."
assert self.page.locator("#stderr-div").text_content() == "one."

  • this is related to the tests above: it tests that we raise an error if we use a non-existent target for output: if we decide to implement output, we should fix this test, and move it somewhere else, because it doesn't make sense to have a test for "warnings and banners" which contains a single test:

    pyscript/pyscript.core/tests/integration/test_warnings_and_banners.py

    Lines 5 to 32 in c6aaacd

    pytest.skip(reason="NEXT: Restore the banner", allow_module_level=True)
    class TestWarningsAndBanners(PyScriptTest):
    # Test the behavior of generated warning banners
    def test_create_singular_warning(self):
    # Use a script tag with an invalid output attribute to generate a warning, but only one
    self.pyscript_run(
    """
    <script type="py" output="foo">
    print("one.")
    print("two.")
    </script>
    <script type="py" output="foo">
    print("three.")
    </script>
    """
    )
    loc = self.page.locator(".alert-banner")
    # Only one banner should appear
    assert loc.count() == 1
    assert (
    loc.text_content()
    == 'output = "foo" does not match the id of any element on the page.'
    )

shadow root?

I have no idea what the following test is testing. It is skipped because Element is gone, but it also seems to test that we attach a shadow root to <py-script>?
My gut feeling is that this is no longer valid with pyscript next and we should just kill the test, but I'll let other people who knows more than me to comment :)

pyscript/pyscript.core/tests/integration/test_shadow_root.py

Lines 7 to 32 in 03c09f1

@pytest.mark.skip("NEXT: Element interface is gone. Replace with PyDom")
def test_reachable_shadow_root(self):
self.pyscript_run(
r"""
<script>
// reason to wait for py-script is that it's the entry point for
// all patches and the MutationObserver, otherwise being this a synchronous
// script the constructor gets instantly invoked at the node before
// py-script gets a chance to initialize itself.
customElements.whenDefined('py-script').then(() => {
customElements.define('s-r', class extends HTMLElement {
constructor() {
super().attachShadow({mode: 'closed'}).innerHTML =
'<div id="shadowed">OK</div>';
}
});
});
</script>
<s-r></s-r>
<script type="py">
import js
js.console.log(Element("shadowed").innerHtml)
</script>
"""
)
assert self.console.log.lines[-1] == "OK"

Other functionalities

On top of that, we have some test_*.py files which are skipped entirely, because their functionality has not implemented yet in NEXT:

  • test_py_terminal.py: the consensus seems to be that we surely want py-terminal back before the final release:

    pyscript/pyscript.core/tests/integration/test_py_terminal.py

    Lines 8 to 15 in c6aaacd

    pytest.skip(
    reason="FIX LATER: pyscript NEXT doesn't support the Terminal yet",
    allow_module_level=True,
    )
    class TestPyTerminal(PyScriptTest):
    def test_py_terminal(self):

  • test_stdio_handling.py: this is very likely related to py-terminal, because in order to implement the terminal, we n ABE7 eed to understand how we want to handle/redirect the standard streams:

    pyscript/pyscript.core/tests/integration/test_stdio_handling.py

    Lines 5 to 9 in c6aaacd

    pytest.skip(reason="NEXT: entire stdio should be reviewed", allow_module_level=True)
    class TestOutputHandling(PyScriptTest):
    # Source of a script to test the TargetedStdio functionality

  • test_py_repl.py: if we have py-terminal, we can easily offer a text-based REPL. The equivalent of the old py-repl will probably be called py-notebook, but I don't know if we want to have it before the final release or not:

    pyscript/pyscript.core/tests/integration/test_py_repl.py

    Lines 7 to 13 in c6aaacd

    pytest.skip(
    reason="NEXT: pyscript NEXT doesn't support the REPL yet",
    allow_module_level=True,
    )
    class TestPyRepl(PyScriptTest):

  • test_splashscreen.py: we discussed this elsewhere, I think we should have a splashscreen when we use pyodide:

    pyscript/pyscript.core/tests/integration/test_splashscreen.py

    Lines 6 to 10 in c6aaacd

    pytest.skip(reason="NEXT: Should we remove the splashscreen?", allow_module_level=True)
    class TestSplashscreen(PyScriptTest):
    def test_autoshow_and_autoclose(self):

@antocuni antocuni mentioned this issue Sep 27, 2023
Closed
@WebReflection
Copy link
Contributor

I think this was done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@WebReflection @antocuni
0