-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
improve test running on mac-os #1710
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1710 +/- ##
=======================================
Coverage 66.60% 66.60%
=======================================
Files 89 89
Lines 15850 15850
Branches 4188 4188
=======================================
Hits 10557 10557
Misses 4203 4203
Partials 1090 1090 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This pull request fixes 6 alerts when merging e011b42 into 8763bed - view on LGTM.com fixed alerts:
|
I'm still getting a few test failures:
|
FYI the two errors about tabs can be fixed by replacing the tabs with spaces in The other error about Docker and MPI being mutually exclusive seems to be hardcoded in the decision tree starting at line 430 of command_line_tool.py I'm not exactly sure what you want to do to fix that. I tried swapping the hints and requirements in
|
e011b42
to
c047adb
Compare
This pull request fixes 6 alerts when merging c047adb into f3a35f3 - view on LGTM.com fixed alerts:
|
c047adb
to
64ed573
Compare
This pull request fixes 6 alerts when merging 64ed573 into f3a35f3 - view on LGTM.com fixed alerts:
|
64ed573
to
2df4452
Compare
This pull request fixes 6 alerts when merging 2df4452 into f3a35f3 - view on LGTM.com fixed alerts:
|
2df4452
to
6e10ec6
Compare
Thanks @jfennick for the testing; alas docker is not included on the GitHub hosted action runners due to licensing issues and the popular community provided action for install docker on macos is not reliably working right now. Can you test this branch one more time with docker on your macos system? |
This pull request fixes 6 alerts when merging 6e10ec6 into f3a35f3 - view on LGTM.com fixed alerts:
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look OK, but I don't have a macos to test it 😞 . I tried testing the GH action, and left a comment in the code review 👍
I saw some tests failing to pull a Docker image with udocker. Didn't spend more time looking into that (Friday 5:30 pm here now, ⛱️ 🥳 ), but that error reminded me of this issue in udocker: indigo-dc/udocker#359
6e10ec6
to
9a62868
Compare
This pull request fixes 6 alerts when merging 9a62868 into 79fd363 - view on LGTM.com fixed alerts:
|
9a62868
to
577cc29
Compare
This pull request fixes 6 alerts when merging 577cc29 into 79fd363 - view on LGTM.com fixed alerts:
|
33b98af
to
369eddd
Compare
This pull request fixes 6 alerts when merging 369eddd into 49fc1c3 - view on LGTM.com fixed alerts:
|
369eddd
to
c308537
Compare
This pull request fixes 6 alerts when merging c308537 into 0e2ced5 - view on LGTM.com fixed alerts:
|
FYI now I'm getting a pytest failure on test_bioconda:
The problem is my .condarc file was
The pytest failure goes away if I add
Perhaps a slightly better error message would be nice, considering the Here are the new and improved error messages for the
Looks like the |
c308537
to
22cd86d
Compare
@jfennick Thanks for the information! I can't find any documentation on why Docker for Desktop, Mac edition adds that directory (and how to turn that off), so I've just hacked around it for now. |
This pull request fixes 6 alerts when merging 22cd86d into 0e2ced5 - view on LGTM.com fixed alerts:
|
Looks like we're just about done. With this change, both pytest and tox work on my machine.
|
22cd86d
to
4b59481
Compare
This pull request fixes 6 alerts when merging 4b59481 into 0e2ced5 - view on LGTM.com fixed alerts:
|
pytest and tox are now both working on my machine. However, I'm still running into #1694 |
and skip another external file test
4b59481
to
bbda773
Compare
This pull request fixes 6 alerts when merging bbda773 into 92c4073 - view on LGTM.com fixed alerts:
|
@jfennick can you test this as well?