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

Add tabulator UI tests #3633

Merged
merged 30 commits into from
Jul 14, 2022
Merged

Add tabulator UI tests #3633

merged 30 commits into from
Jul 14, 2022

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Jun 17, 2022

Starts to address #3631

Very much WIP at this stage, I'm getting used to PlayWright's API and developing a workflow to write these tests.

  • The UI test suite runs by default on Chromium only. I guess this is fine for now, I've just made that more explicit by declaring it in the command line options and by only installing Chromium in the CI.
  • I've made use of the expect function that makes assertions while waiting, this is I believe more robust than using time.sleep()
  • I've added a small wait_until function that I use after triggering an event on the UI side and waiting for an effect on the Python side, this should be more robust that using time.sleep().
  • I'll try to use as much as possible the text locators (i.e. page.locator('text="blabla"')) as they avoid to have to rely on internal details. Yet they can't always be used.

As for my workflow to write these scripts, I combine two approaches. First run a script with python script.py that opens a Panel app and the Playwright Inspector that I've found useful to get used to the selectors and to record some actions (it generates the corresponding Python code automatically). Here's an example of the script I use:

import time
import panel as pn
from panel.widgets import Tabulator
from pandas._testing import makeMixedDataFrame
pn.extension('tabulator')

df = makeMixedDataFrame()
df.index = [f'idx{i}' for i in range(len(df))]
table = Tabulator(df, selectable=True)

port=5009
pn.serve(table, threaded=True, port=port, show=False)
time.sleep(1)

from playwright.sync_api import sync_playwright
playwright = sync_playwright().start()
browser = playwright.chromium.launch(headless=False)
page = browser.new_page()
page.goto(f"http://localhost:{port}")
page.pause()

In the second approach I open a Jupyter Notebook and execute this code:

import panel as pn
from panel.widgets import Tabulator
from pandas._testing import makeMixedDataFrame
pn.extension('tabulator')

df = makeMixedDataFrame()
df.index = [f'idx{i}' for i in range(len(df))]
table = Tabulator(df, selectable=True)

port=5010
pn.serve(table, threaded=True, port=port, show=False)

And this code that makes use of the async API offered by Playwright to control the browser.

from playwright.async_api import async_playwright
playwright = await async_playwright().start()
browser = await playwright.chromium.launch(headless=False)
page = await browser.new_page()
await page.goto(f"http://localhost:{port}")

With this second approach I can easily explore Playwright's API.

Writing tests that cover all the behaviors offered by the widget is going to be a lot of work but is also going to be definitively worth it in the long run.

@codecov
Copy link

codecov bot commented Jun 18, 2022

Codecov Report

Merging #3633 (a97913f) into master (1790cc0) will increase coverage by 1.64%.
The diff coverage is 97.65%.

@@            Coverage Diff             @@
##           master    #3633      +/-   ##
==========================================
+ Coverage   81.90%   83.55%   +1.64%     
==========================================
  Files         206      207       +1     
  Lines       28067    29642    +1575     
==========================================
+ Hits        22989    24767    +1778     
+ Misses       5078     4875     -203     
Flag Coverage Δ
ui-tests 31.88% <95.56%> (?)
unitexamples-tests 78.22% <12.59%> (?)

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

Impacted Files Coverage Δ
panel/widgets/tables.py 89.11% <0.00%> (+1.18%) ⬆️
panel/tests/util.py 86.45% <77.41%> (+1.84%) ⬆️
panel/tests/ui/widgets/test_tabulator.py 98.06% <98.06%> (ø)
panel/tests/conftest.py 95.65% <100.00%> (+1.38%) ⬆️
panel/tests/widgets/test_tables.py 99.65% <100.00%> (+0.01%) ⬆️
panel/viewable.py 66.77% <0.00%> (+0.31%) ⬆️
panel/pane/base.py 87.91% <0.00%> (+0.41%) ⬆️
panel/util.py 86.36% <0.00%> (+0.75%) ⬆️
panel/reactive.py 77.65% <0.00%> (+0.77%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1790cc0...a97913f. Read the comment docs.

@thuydotm thuydotm mentioned this pull request Jun 27, 2022
@thuydotm
Copy link
Collaborator

@maximlt why don't we add env setup for running test with PlayWright in a separate PR and merge it into master? That would be convenient for future PRs that add new UI tests to run on GitHub Actions without duplicating the setup steps from this PR.

@maximlt maximlt marked this pull request as draft June 28, 2022 11:50
@maximlt
Copy link
Member Author

maximlt commented Jun 28, 2022

What do you mean by env setup exactly? I should have marked this PR as Draft from the beginning as it's still WIP and I'm currently writing more tests. I should merge it soon though.

@thuydotm
Copy link
Collaborator

I see. By env setup, I meant the changes to tox.ini and .github/workflows/test.yaml.

@maximlt
Copy link
Member Author

maximlt commented Jun 28, 2022

Ok I see. I think the changes I've made are just to try to be more explicit in the way the UI tests are setup (e.g. just installing Chromium) but don't actually change anything in how the tests run. I've tried to fix the coverage going down on this PR with --cov=./panel but that obviously failed, I'll have to keep investigating. So I think as for the env setup there isn't anything you need to re-use in other PRs. Let me know if I missed something!

@philippjfr
Copy link
Member

Yeah I don't think those changes are particularly consequential just saves installing other browsers pointlessly.

@maximlt maximlt marked this pull request as ready for review July 14, 2022 14:11
@maximlt
Copy link
Member Author

maximlt commented Jul 14, 2022

Merging, despite a couple of flaky tests failing as they're not part of the new tests added in this PR. I wouldn't be surprised however to see that this PR introduces more flaky tests... The last runs were pretty robust, I'll watch the test suite over the next runs to see if it uncovers some.

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.

3 participants