add test coverage workflow

[danieleades]

No description provided.

[codecov-commenter]

Welcome to Codecov 🎉

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 ☂️

[Philippe-Cholet]

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]

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]

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

[jswrenn]

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

itertools/.github/workflows/ci.yml

Lines 3 to 9 in f60fe7e

	on:
	pull_request:
	paths-ignore:
	- "**.md"
	merge_group:
	paths-ignore:
	- "**.md"

[danieleades]

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

itertools/.github/workflows/ci.yml

Lines 3 to 9 in f60fe7e

	on:
	pull_request:
	paths-ignore:
	- "**.md"
	merge_group:
	paths-ignore:
	- "**.md"

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]

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:

itertools/.github/workflows/ci.yml

Lines 3 to 9 in eeda182

	on:
	pull_request:
	paths-ignore:
	- "**.md"
	merge_group:
	paths-ignore:
	- "**.md"

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]

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]

i've updated based on the discussion in this issue - matplotlib/napari-matplotlib#155

[Philippe-Cholet]

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]

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

[jswrenn]

LGTM! If, for some reason, this isn't quite the right combination of triggers, we can revise in a follow-up PR.

Thank you so much for this!

[Philippe-Cholet]

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

[danieleades]

#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:

• upload tokens are required for private repos
• upload tokens are not required for public repos, since the public api is used
• the public API is rate-limited which occasionally causes the job to fail
• adding the codecov token to the action in a public repo obviates the problem

[Philippe-Cholet]

I searched a bit and found the same thing: codecov/codecov-action#557 (comment)

[danieleades]

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]

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]

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

• EitherOrBoth miss quite some doctests if we were diligent enough about all the methods.
• Combinations::reset has some unused code (because Powerset only uses increasing ks).
• MultiProduct::count would be tested with Rewrite MultiProduct #835.
• Very few size_hint/next_back untested. Specialization tests are not full yet.
• A few edge cases here and there.