posit-dev / great-tables

Make awesome display tables using Python.
https://posit-dev.github.io/great-tables/
MIT License
1.43k stars 48 forks source link

fix: issues with save function #276

Closed rich-iannone closed 1 month ago

rich-iannone commented 2 months ago

This adds a workflow job that tests whether the .save() method is properly producing a table image when using the "firefox" webdriver.

Also fixes: https://github.com/posit-dev/great-tables/issues/241

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.71%. Comparing base (ed09f9d) to head (c4bae56). Report is 27 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #276 +/- ## ========================================== - Coverage 81.72% 81.71% -0.01% ========================================== Files 41 41 Lines 4323 4321 -2 ========================================== - Hits 3533 3531 -2 Misses 790 790 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rich-iannone commented 2 months ago

This fails with:

python .github/scripts/save_browser_table.py
Traceback (most recent call last):
  File "/home/runner/work/great-tables/great-tables/.github/scripts/save_browser_table.py", line 3, in <module>
    GT(exibble).save("exibble_firefox.png", web_driver="firefox")
  File "/home/runner/work/great-tables/great-tables/great_tables/_export.py", line 181, in save
    wdriver(options=wd_options) as headless_browser,
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/selenium/webdriver/firefox/webdriver.py", line 69, in __init__
    super().__init__(command_executor=executor, options=options)
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/selenium/webdriver/remote/webdriver.py", line 208, in __init__
    self.start_session(capabilities)
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/selenium/webdriver/remote/webdriver.py", line 292, in start_session
    response = self.execute(Command.NEW_SESSION, caps)["value"]
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/selenium/webdriver/remote/webdriver.py", line 347, in execute
    self.error_handler.check_response(response)
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/selenium/webdriver/remote/errorhandler.py", line 229, in check_response
    raise exception_class(message, screen, stacktrace)
selenium.common.exceptions.WebDriverException: Message: Process unexpectedly closed with status 1

make: *** [Makefile:53: save-browser-table] Error 1

Probably because Firefox is run in non-headless mode: https://stackoverflow.com/questions/46809135/webdriver-exceptionprocess-unexpectedly-closed-with-status-1

Now, technically it runs in headless mode on my system but the resulting image is incorrect (only gives correct image output when not run in headless mode).

rich-iannone commented 2 months ago

I suppose we could instead run with chrome using: https://github.com/browser-actions/setup-chrome.

machow commented 2 months ago

Ah, dang--what does the image look like? It looks there are options to run FF headless in that SO post and this one, so I'm curious if there's a way to get it to work?!

rich-iannone commented 2 months ago

The firefox screenshot looks like this in headless mode:

test-firefox

and otherwise just fine if we run w/o headless:

test-firefox_not_headless

I'll try out some of the things in that SO post to see if there's a way to work around this!

rich-iannone commented 2 months ago

Seems like "---headless=new" solved the problem! See https://www.selenium.dev/blog/2023/headless-is-going-away/. I'll test this with the rest of the web drivers to ensure everything works with that.

rich-iannone commented 2 months ago

With "---headless=new" all four browsers now work well in headless mode. Pushed the change.

machow commented 1 month ago