search

rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.69k stars 303 forks source link

add test coverage workflow #847

Closed danieleades closed 7 months ago

codecov-commenter commented 7 months ago

Welcome to Codecov :tada:

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered :open_umbrella:

Philippe-Cholet commented 7 months ago

After reading the readme, I learnt that nightly is required to support doctests (and without it, coverage would probably be quite lacking for non-adaptor Itertools methods, as I found out when experimenting with tarpaulin a while back). Looks good otherwise.

danieleades commented 7 months ago

After reading the readme, I learnt that nightly is required to support doctests (and without it, coverage would probably be quite lacking for non-adaptor Itertools methods, as I found out when experimenting with tarpaulin a while back). Looks good otherwise.

i'm not sure what you mean? Nightly is required for the doctests coverage, which is why that's the toolchain used in the github action. Is that an issue?

Philippe-Cholet commented 7 months ago

Not an issue but an observation that I found useful to share, that's all. Looks good otherwise.

danieleades commented 7 months ago

Shouldn't the triggers for this be basically identical to our triggers for CI testing?

https://github.com/rust-itertools/itertools/blob/f60fe7e9548ffb047420c3d9ec576aa792d8e9ca/.github/workflows/ci.yml#L3-L9

sure, i can update with this. it will also need to run on master though, so would be something like (i guess)-

 on: 
   pull_request: 
     paths-ignore: 
       - "**.md" 
  push: 
    branches: [ master ]
     paths-ignore: 
       - "**.md" 
   merge_group: 
     paths-ignore: 
       - "**.md"

that look right?

Philippe-Cholet commented 7 months ago

We currently only "push changes to master" by adding PRs to the merge queue (merge_group trigger) so I think your on: push: branches: [master] is quite similar to on: merge_queue:, right? (Or am I missing something? github workflow noob here). So it could just be the same as in CI:

https://github.com/rust-itertools/itertools/blob/eeda182b9dcc55bb89c6cedba95e04cc0f6bc384/.github/workflows/ci.yml#L3-L9

Maybe the difference is that on: push: branches: [master] happens "when merged" while on: merge_queue: is "while being merged"? I'm not sure to care about this difference.

danieleades commented 7 months ago

I'm not at all familiar with the intersection of merge queues and GitHub actions.

For codecov to do its thing it needs to run on PRs and also needs to have run on the most recent commit on master at any one time (for comparison). So long as any squashing/merging is done before the merge queue does its thing I guess you don't also need to run on push? The main thing is that the commit hash is identical to the tip of master

danieleades commented 7 months ago

i've updated based on the discussion in this issue - https://github.com/matplotlib/napari-matplotlib/issues/155

Philippe-Cholet commented 7 months ago

Since merge_group is not a trigger anymore (not in on: block), I am not sure if: github.event_name != 'merge_group' is useful but it sure is not a problem to have it. Seems good to me.

danieleades commented 7 months ago

Since merge_group is not a trigger anymore (not in on: block), I am not sure if: github.event_name != 'merge_group' is useful but it sure is not a problem to have it. Seems good to me.

i'll happily remove it if it's redundant. I don't know have any experience with github merge queues

Philippe-Cholet commented 7 months ago

https://github.com/rust-itertools/itertools/pull/835#issuecomment-1898863835 says "Missing Base Commit". Maybe that merge this did not trigger an initial coverage report?

danieleades commented 7 months ago

#835 (comment) says "Missing Base Commit". Maybe that merge this did not trigger an initial coverage report?

looks like this coverage job ran on master but failed to upload - https://github.com/rust-itertools/itertools/actions/runs/7573294610/job/20625097590

edit: looks like a rate-limiting issue from the github API. adding an upload token is described as a workaround. specifically:

Philippe-Cholet commented 7 months ago

I searched a bit and found the same thing: https://github.com/codecov/codecov-action/issues/557#issuecomment-1224970469

danieleades commented 7 months ago

the codecov token secret will need to be set up by a maintainer alternatively, a maintainer could trigger the workflow again on master

Philippe-Cholet commented 7 months ago

For the secret, I'm pretty sure an owner would be required, which means jswrenn (or bluss). As a temporary fix, I triggered the workflow and got success: https://app.codecov.io/github/rust-itertools/itertools/tree/master/src

Philippe-Cholet commented 7 months ago

We don't test Debug/Display/Clone implementations (and helper macros). Apart from this, it's pretty good: