Fix autoreload

[hoxbro]

Fixes #5489

self.count has a default of None (meaning unlimited), which will always give an exception out when comparing to self.counter

[codecov]

Codecov Report

Merging #5490 (129827e) into main (36a1d92) will decrease coverage by 11.03%.
Report is 3 commits behind head on main.
The diff coverage is 45.83%.

❗ Current head 129827e differs from pull request most recent head 92c12c4. Consider uploading reports for the commit 92c12c4 to get more accurate results

@@             Coverage Diff             @@
##             main    #5490       +/-   ##
===========================================
- Coverage   83.29%   72.27%   -11.03%     
===========================================
  Files         275      276        +1     
  Lines       40124    40135       +11     
===========================================
- Hits        33423    29006     -4417     
- Misses       6701    11129     +4428     

Flag	Coverage Δ
ui-tests	?
unitexamples-tests	72.27% <45.83%> (-0.31%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
panel/tests/ui/command/test_serve.py	43.47% <43.47%> (ø)
panel/io/callbacks.py	61.31% <100.00%> (-19.13%)	⬇️

... and 85 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[maximlt]

Is it possible to test this? Breaking --autoreload is a pretty bad regression :(

[hoxbro]

Is it possible to test this?

I quickly tried to do something with subprocess but found it difficult to get up and running. Having a type checker running (which works with Param) could have detected this error.

Breaking --autoreload is a pretty bad regression :(

Yep, and it is all PeriodicCallback, which does not have a count set that does not work. I would suggest we release a micro-release at the start of next week with this fix in it.

[philippjfr]

Yes, let's release 1.2.3 asap. Definitely need a test here.

[hoxbro]

I have been trying to get a test to run with the following, which works fine if I use something like: pytest panel/tests --ui -k test_autoreload_app --slowmo but not when I remove --slowmo.

import os

import pytest
import time

from panel.tests.util import (
    run_panel_serve, unix_only, wait_for_port, write_file
)

try:
    from playwright.sync_api import expect
    pytestmark = pytest.mark.ui
except ImportError:
    pytestmark = pytest.mark.skip('playwright not available')


@unix_only
def test_autoreload_app(py_file, port, page):
    app = "import panel as pn; pn.Row('Example 1').servable()"
    app2 = "import panel as pn; pn.Row('Example 2').servable()"
    write_file(app, py_file.file)

    app_name = os.path.basename(py_file.name)[:-3]

    with run_panel_serve(["--port", str(port), '--autoreload', py_file.name]) as p:
        port = wait_for_port(p.stdout)
        time.sleep(0.2)

        page.goto(f"http://localhost:{port}/{app_name}")
        expect(page.locator(".markdown")).to_have_text("Example 1")

        write_file(app2, py_file.file)
        expect(page.locator(".markdown")).to_have_text('Example 2')

[philippjfr]

Can you add the test to the PR, then I'll iterate.

[hoxbro]

Done. I want to note we can't use page.goto after write_file as this also worked without this fix.

[philippjfr]

Added a timeout, which isn't elegant but should work. The problem here is that I guess is that the file is changed before the websocket is fully open, which means it never receives the event.

Wonder if maybe we should put a Document.hold() while the app is initializing and then unhold when the DocumentReady event fires.

[hoxbro]

I feel like I tried all combinations of time.sleep and page.wait_until_timeout to get this to work. Apparently, I did not...

Thank you for the solution 👍

[philippjfr]

If this passes I'll merge as is, because the .hold() + .unhold() thing seems like a good idea but has the potential to break a ton of stuff, so should be tackled separately.