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

Remove asynchronous=False from runJavaScript calls #84

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dhomeier
Copy link
Contributor

@dhomeier dhomeier commented Mar 25, 2022

Description

Currently causing failures

>       self.viewer._wwt.widget.page.runJavaScript("tourxml = '';", asynchronous=False)
E       TypeError: runJavaScript() got an unexpected keyword argument 'asynchronous'

on all Python 3.7+ tests although all the Qt* modules seem to be on the same versions as in the 3.6 envs.
Search of the QtWebEngine docs did not bring up anything about a deprecation or potential replacement, rather appears it is now running asynchronously by default, so just giving this a try.

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #84 (3298584) into main (608d879) will increase coverage by 1.27%.
The diff coverage is 100.00%.

❗ Current head 3298584 differs from pull request most recent head 5774409. Consider uploading reports for the commit 5774409 to get more accurate results

@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
+ Coverage   71.97%   73.25%   +1.27%     
==========================================
  Files          18       18              
  Lines         860      860              
==========================================
+ Hits          619      630      +11     
+ Misses        241      230      -11     
Impacted Files Coverage Δ
glue_wwt/viewer/tools.py 71.69% <100.00%> (+20.75%) ⬆️
glue_wwt/viewer/tests/test_wwt_widget.py 97.47% <0.00%> (ø)

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 608d879...5774409. Read the comment docs.

@astrofrog
Copy link
Member

@dhomeier is this still needed? can you rebase?

@dhomeier
Copy link
Contributor Author

@dhomeier is this still needed? can you rebase?

Attempting to run without asynchronous=False is obviously not a solution, but restoring the kwarg and the test test_save_tour now fails on all CI platforms. So this particular PR is probably of no further use; could just leave #85 open.
But I think that means SaveTourTool now has to be considered nonfunctional with any more recent JS installations.

@dhomeier
Copy link
Contributor Author

dhomeier commented May 20, 2022

There may be some useful hints in https://stackoverflow.com/a/60831141 for implementing an alternative.

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.

2 participants