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 CI, tox config and other test adjustments #3

Merged
merged 26 commits into from
May 4, 2023
Merged

Add CI, tox config and other test adjustments #3

merged 26 commits into from
May 4, 2023

Conversation

jaimergp
Copy link
Contributor

I had not imported the full history in my previous attempt. Restarting here.

Supersedes #1

@jaimergp jaimergp mentioned this pull request Apr 25, 2023
@Czaki
Copy link
Contributor

Czaki commented Apr 25, 2023

So if test requires dialogs then in pytest config there is a need to add new marker:
like here:
https://github.com/4DNucleome/PartSeg/blob/17eb8128e65f099536ac3edf2394b17b9b03787c/setup.cfg#L156

and decorate this test with @pytest.mark.enabledialog

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@9fe2e15). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #3   +/-   ##
=======================================
  Coverage        ?   90.56%           
=======================================
  Files           ?        5           
  Lines           ?     1166           
  Branches        ?        0           
=======================================
  Hits            ?     1056           
  Misses          ?      110           
  Partials        ?        0           

@jaimergp
Copy link
Contributor Author

The tests on napari_plugin_manager/_tests/test_qt_plugin_dialog.py are flaky on macOS. Sometimes they pass, but most often they don't. The error is usually:

AttributeError: 'NoneType' object has no attribute 'action_button'

with None being widget, as given by a line above:

widget = plugin_dialog_constructor.available_list.itemWidget(item)

Is this a known bug, issue or limitation? Any workarounds? It looks like it might be a race condition?

cc @goanpeca

@jaimergp
Copy link
Contributor Author

After discussing the issue with the team on the Europe community meeting, I've added the relevant waitUntil calls to make up for the threads/scheduler timing issues.

What we are seeing now:

 >       qtbot.waitUntil(lambda: widget.available_list.count() >= 2, timeout=300)
  E       pytestqt.exceptions.TimeoutError: waitUntil timed out in 300 milliseconds
  napari_plugin_manager\_tests\test_qt_plugin_dialog.py:183: TimeoutError

This one happens in:

  • Windows, Python 3.9, PySide2
  • Windows, Python 3.9, PyQt5
  • Windows, Python 3.10, PySide6
  • macOS, Python 3.9, PyQt5

No pattern is obvious, I might just crank the timeout a bit longer?


  >       with qtbot.waitSignal(installer.allFinished, timeout=10000):
  E       pytestqt.exceptions.TimeoutError: Signal allFinished() not emitted after 10000 ms
  
  napari_plugin_manager\_tests\test_installer_process.py:149: TimeoutError

This one happens in:

  • Windows, Python 3.10, PyQt5
  • macOS, Python 3.10, PySide6

Network error?

@jaimergp
Copy link
Contributor Author

With 1000ms of timeout, all Windows runners passed, but on macOS we observe timeouts on:

  • py3.9, PyQt5
  • py3.10, PyQt5
  • py3.11, PyQt6

Maybe a bit more will fix it? The Python 3.10 and 3.11 errors were not present before, so I assume they are happening at random.

@jaimergp
Copy link
Contributor Author

Ok, finally green! 🚀

@jaimergp jaimergp marked this pull request as ready for review April 26, 2023 23:03
@jaimergp jaimergp requested review from goanpeca and Czaki April 26, 2023 23:03
qtbot.add_widget(widget)
qtbot.waitUntil(lambda: widget.available_list.count() >= 2, timeout=300)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we know how many entries we expect here? If the test ends before the thread yelding new plugin entries it may end with a crash whole test setup. We should wait until this thread ends working.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also lambdas are sometimes funky in these tests, so using a separate function might be slightly better

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

@Czaki
Copy link
Contributor

Czaki commented Apr 27, 2023

For reference. Bug pointed on community meeting also happens in the main repository https://github.com/napari/napari/actions/runs/4824544130/jobs/8594439707?pr=5784

@jaimergp
Copy link
Contributor Author

For reference. Bug pointed on community meeting also happens in the main repository https://github.com/napari/napari/actions/runs/4824544130/jobs/8594439707?pr=5784

Yay not feeling alone anymore :D

@jaimergp jaimergp requested a review from Czaki May 2, 2023 09:37
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@Czaki Czaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small comments/changes. Everything else looks good.

napari_plugin_manager/_tests/test_qt_plugin_dialog.py Outdated Show resolved Hide resolved
@jaimergp jaimergp merged commit 76e8725 into main May 4, 2023
@jaimergp jaimergp deleted the add-ci branch May 4, 2023 09:05
@jaimergp
Copy link
Contributor Author

jaimergp commented May 4, 2023

Thanks for the super helpful review @Czaki! I learnt a lot about Qt, operating systems and event loops!

@goanpeca goanpeca added the task Work around tooling / release / ci label Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
task Work around tooling / release / ci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants