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

Add gcov coverage testing for C code #457

Merged
merged 3 commits into from Apr 7, 2021
Merged

Conversation

bwoodsend
Copy link
Collaborator

Fixes #387.

Changes proposed in this pull request:

  • Add a shell script for running gcov on either Linux or macOS.
  • Run said shell script on CI/CD and upload the results to codecov.

Here is a CI run with it. I'm guessing that this error is because I'm running from my own fork?

And I ended up having to drop colored reports on Linux because it turns out it's too new a feature to be available on Github's older Ubuntu VMs.

@bwoodsend
Copy link
Collaborator Author

I love how quick your CI is btw. Over on PyInstaller it's 8 hours a run with no parallelizing if two people submit a PR within 8 hours of each other.

@bwoodsend bwoodsend marked this pull request as ready for review April 6, 2021 19:49
@bwoodsend
Copy link
Collaborator Author

I was only waiting for your confirmation here. So ideally the testing with coverage whoo hah would have just been conditionally adding:

env:
  CFLAGS: '-coverage -Og'

to the pip install . step but because of said kink we have to rebuild with pip install -e . and run again so the test suite now gets run twice but given that it all runs in a few seconds I doubt that matter really.

@hugovk
Copy link
Member

hugovk commented Apr 6, 2021

I'm guessing that this error is because I'm running from my own fork?

As far as I'm aware, Codecov has not been used on this org or repo before. I've checked https://app.codecov.io/gh/ultrajson/ultrajson/ and it says no action needed, but also to set a token:

untitled

Is it so a token is required for the bash uploader? Did you set one in your fork?

@bwoodsend
Copy link
Collaborator Author

Oh, I assumed you already had one so I didn't try to set one up. I just let codecov upload fail on hitting the authentication error when I ran it locally. Shall I create a token then send it to you once I get it working or would you rather do it?

@bwoodsend
Copy link
Collaborator Author

bwoodsend commented Apr 6, 2021

Actually it doesn't look like I have the permissions to do it anyway. I can set it up for my own fork but it'll only be measuring my fork and published under bwoodsend/ultrajson which isn't much use.

@hugovk
Copy link
Member

hugovk commented Apr 6, 2021

I've added the CODECOV_TOKEN as a repo secret at https://github.com/ultrajson/ultrajson/settings/secrets/actions.

image

Shall we rerun the CI?

@bwoodsend
Copy link
Collaborator Author

Hmm, it's supposed to be automatic but failing that, I think I'll need to add a line to turn the secret into an environment variable.

@bwoodsend
Copy link
Collaborator Author

Give it a re-run - see what happens...

It isn't supported on the older gcc versions used by CI/CD.
@bwoodsend
Copy link
Collaborator Author

Oh it did it!

@bwoodsend
Copy link
Collaborator Author

bwoodsend commented Apr 6, 2021

alt text

Not entirely sure what this graph is trying to say but at least it's there.

@bwoodsend
Copy link
Collaborator Author

I'm surprised that worked though. I thought PRs weren't supposed to be able to use their target repo's secrets.

@hugovk
Copy link
Member

hugovk commented Apr 7, 2021

Excellent, let's merge!

88.64% is good for a first measurement!

Yep, those charts aren't so obvious. I believe it's a visualisation of how much is covered in the root, and then each subdir, and then each file. I don't usually pay much attention. The greener the better!

Thank you!

@hugovk hugovk merged commit c40a9b4 into ultrajson:master Apr 7, 2021
@hugovk hugovk added the changelog: Added For new features label Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Added For new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test with coverage
2 participants