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

setup coverage.py in tox env #217

Merged
merged 7 commits into from
Apr 22, 2021
Merged

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Apr 15, 2021

Hopefully this satisfies the first part of #200 (comment). If so, I'll move on to adding reports to Github actions, etc. Thanks!

@H-Shay
Copy link
Contributor Author

H-Shay commented Apr 15, 2021

Noting that the towncrier github actions check is failing. Looks like an error with a config file somewhere (git produced output while failing:
fatal: ambiguous argument 'origin/master...': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git [...] -- [...]')
I didn't experience this error when setting up the github actions so I'm a little perplexed, but I will look deeper into it tomorrow.

@anoadragon453
Copy link
Member

The command that towncrier is running is git diff --name-only origin/master..., and failing to figure out what origin/master is. There's a lot of possible reasons for this. checkout@v2 might be specifying your fork as origin, and may only be checkout out your add-coverage branch but nothing else. It may be possible to fix this by just doing a git fetch origin before running towncrier.

I'd probably play around in this PR (or another) and try to get some information out of GitHub Actions in terms of how it's set it's remotes and branches up.

@callahad
Copy link
Contributor

Looks good!

  • Let's add .coverage* and htmlcov/ to the .gitignore file and remove them from this pull request; no need to ship the reports with our source.
  • It would probably be good to run coverage report --sort=cover and coverage html so that there's something in the console as well as the html if you want to delve deeper.
  • ...and maybe only run the report(s) if the initial -m twisted.trial succeeds?

@callahad
Copy link
Contributor

As to the towncrier bit... by default GitHub Actions does a shallow clone of the repo which has only one commit: the result of the proposed merge. So there's no actual history there to compare against. Solving that properly is a bit of a rabbit hole, but I think setting:

- uses: actions/checkout@v2
  with:
    fetch-depth: 0
    ref: ${{ github.event.pull_request.head.sha }}

would get you a full history, check out the actual branch head (not the proposed merge) and allow the check to work.

@H-Shay
Copy link
Contributor Author

H-Shay commented Apr 15, 2021

Ok cool I will open a different PR for the towncrier issue and see if I can't get it working using the insight you both offered.

I've updated the PR to add the other changes related to coverage. However I have one question:

In terms only running the coverage reports if the -m twisted.trial succeeds, I am wondering if tox doesn't do this automatically? Looking at the docs, under commands it states that "Commands will execute one by one in sequential fashion until one of them fails (their exit code is non-zero) or all of them succeed." My understanding of this is that tox will exit if the twisted.trial fails. I am unfamiliar with tox so please correct me if I'm wrong!

@callahad
Copy link
Contributor

"Commands will execute one by one in sequential fashion until one of them fails (their exit code is non-zero) or all of them succeed."

Oh, cool! You're probably totally good there, then :)

@H-Shay
Copy link
Contributor Author

H-Shay commented Apr 16, 2021

Great then this should be all set to go!

@anoadragon453 anoadragon453 requested a review from a team April 16, 2021 18:07
@richvdh richvdh requested review from callahad and removed request for a team April 19, 2021 13:27
@richvdh
Copy link
Member

richvdh commented Apr 19, 2021

@callahad I think you have most context here so if you have the cycles to pick up the review that would be appreciated!

Copy link
Contributor

@callahad callahad left a comment

Choose a reason for hiding this comment

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

Great, thanks! Could you please:

  1. Merge origin/master (or rebase onto origin/master) so we can get working towncrier checks
  2. Add coverage~=5.5 as a dependency somewhere to ensure that a cleantox virtual environment has access to it? (e.g., it could be a dev dependency in setup.py, or a one-off deps entry in the tox.ini testenv.

@H-Shay
Copy link
Contributor Author

H-Shay commented Apr 21, 2021

Alright I think that did it. Thank you!

@H-Shay H-Shay requested a review from callahad April 21, 2021 19:12
@callahad
Copy link
Contributor

We're almost good :) The tox.ini still needs to be adjusted to actually install coverage in the default testenv. You can just remove the extras = stanza from the other environments and put it straight in the base testenv and then all three will inherit it and install the dev dependencies.

@H-Shay
Copy link
Contributor Author

H-Shay commented Apr 21, 2021

Thanks for the hand-holding, I am actually learning a lot. Hopefully this is the last update!

@callahad
Copy link
Contributor

Thank you so much!

@callahad
Copy link
Contributor

Oh, hey, could you sign-off on a commit in this branch, please?

You can do a git commit --signoff --allow-empty to make an empty commit to that effect :)

@callahad callahad merged commit b389bc6 into matrix-org:master Apr 22, 2021
awesome-michael added a commit to Awesome-Technologies/sygnal that referenced this pull request May 6, 2021
Sygnal 0.9.3 (2021-04-22)
=========================

Features
--------

- Prevent the push key from being rejected for temporary errors and oversized payloads, add TTL logging, and support `events_only` push data flag. ([\matrix-org#212](matrix-org#212))
- WebPush: add support for Urgency and Topic header ([\matrix-org#213](matrix-org#213))

Bugfixes
--------

- Fix a long-standing bug where invalid JSON would be accepted over the HTTP interfaces. ([\matrix-org#216](matrix-org#216))
- Limit the size of requests received from HTTP clients. ([\matrix-org#220](matrix-org#220))

Updates to the Docker image
---------------------------

- Remove manually added GeoTrust Root CA certificate from docker image as Apple is no longer using it. ([\matrix-org#208](matrix-org#208))

Improved Documentation
----------------------

- Make `CONTIBUTING.md` more explicit about how to get tests passing. ([\matrix-org#188](matrix-org#188))
- Update `CONTRIBUTING.md` to specify how to run code style and type checks with Tox, and add formatting to code block samples. ([\matrix-org#193](matrix-org#193))
- Document how to work around pip installation timeout errors. Contributed by Omar Mohamed. ([\matrix-org#215](matrix-org#215))

Internal Changes
----------------

- Update Tox to run in the installed version of Python (instead of specifying Python 3.7) and to consider specific paths and folders while running checks, instead of the whole repository (potentially including unwanted files and folders, e.g. the virtual environment). ([\matrix-org#193](matrix-org#193))
- Make development dependencies available as extras. Contributed by Hillery Shay. ([\matrix-org#194](matrix-org#194))
- Update `setup.py` to specify that a minimum version of Python greater or equal to 3.7 is required. Contributed by Tawanda Moyo. ([\matrix-org#207](matrix-org#207))
- Port CI checks to Github Actions. ([\matrix-org#210](matrix-org#210), [\matrix-org#219](matrix-org#219))
- Upgrade development dependencies. Contributed by Omar Mohamed ([\matrix-org#214](matrix-org#214))
- Set up `coverage.py` to run in tox environment, and add html reports ([\matrix-org#217](matrix-org#217))

Change-Id: I14ae821ff2a1562e91fd87ce25f73baaa0b9430b
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.

4 participants