-
Notifications
You must be signed in to change notification settings - Fork 77
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 github action for tests #623
Conversation
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.
It's great to see that we can make progress here! Some suggestions regarding to your changes:
- Please see my inline comment so we can have a broader coverage across differen Python releases.
- Also remove the configurations for Travis CI and AppVeyour which are not necessary anymore given that GitHub actions now handle all platforms.
- If we can extend the timeout for the remote tinderbox tests then I'm happy with a change there as well. Otherwise feel free to mark them as skipped for now so that we have a passing test suite. We can check for improvements in a follow-up issue.
Thanks!
6d58c3a
to
7f50bc5
Compare
4629c17
to
996ac23
Compare
Thanks for the suggestion. I have added more python version for The tests are failing on Python 3.10 due to |
Other possible improvements: |
That's great! Thanks a lot for the update!
It would probably be enough for now to only use a single Python version for MacOS and Windows. Maybe we use the last official release for it?
Would you mind filing an issue for that at https://github.com/web-platform-tests/wpt/issues and referencing here and the yml file? We might have to ignore Python 3.10 for now until it got fixed.
Lets do that. But as I can read in the GitHub documentation it would require a
Please add whatever you can to the appropriate files. Once the PR is merged I can update the repository settings. |
Hey @whimboo, the PR is ready for review. |
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.
This looks great. Just two remaining questions. Once these are solved I would land the code and setup the repository to handle the dependabot and coveralls services.
Please also update the README.md file and exchange the pyup badge with dependabot as well.
Dependabot doesn't seem to have a badge yet dependabot/dependabot-core#1912, so I will remove the pyup badge. |
6daabc8
to
9cf7dbe
Compare
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.
Thanks a lot for the last update! There is just one small thing and then we seem to be ready to get this PR merged.
Created the issue #625, once the PR is merged, I will update the description with the |
Thanks for filing the issue. I've updated the skip reason with a link to the newly filed issue. If tests are all passing I'm going ahead and merge this PR. |
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.
Tests are all passing. As such lets get this landed and if something is still missing we can follow-up on. Again, thanks a lot for working on this PR and bringing the project back to life!
This PR adds github actions for tests, resolves #614.
It currently fails for
tests/remote/test_firefox.py::test_tinderbox_scraper
test as for some arguments the request timesout (Given enough time it returns HTTP 504).It is also happening locally.
Logs - https://github.com/mozilla/mozdownload/actions/runs/3041139681/jobs/4897959420#step:5:366
Let me know if you have suggestions.