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

Simple Code Coverage analysis #378

Merged
merged 19 commits into from
Feb 5, 2021
Merged

Conversation

mattpolzin
Copy link
Contributor

Closes #350.

Adds simple code coverage analysis. Until the GitHub token is set up in this repo, you can see the results including a comment on the PR here: mattpolzin#2

I only commented out the tests to make the coverage diff with main interesting. Obviously not expecting that change to get merged :)

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Feb 1, 2021

This is awesome, thanks! I wonder if coverage diffs could be posted as a GitHub Action logging command? Then it wouldn't require any tokens for comments. It would look similar to this, visible only in diffs of PRs:

screenshot

According to the GitHub Actions docs it's enough to output a line in this form to stdout on a build agent for it to be displayed this way: ::warning file=Package.swift,line=1,col=1::Code coverage summary goes here.... The SwiftLint action formats its output in a similar way.

What do you think about this approach?

@MaxDesiatov MaxDesiatov added the continuous integration Changes related to the continuous integration process label Feb 1, 2021
@mattpolzin
Copy link
Contributor Author

@MaxDesiatov That's an interesting idea. Makes me think it would be really cool to tag the appropriate Swift file's line 1 when that file has low test coverage (or has decreased test coverage when compared to main) but that's just an idea, not something easy to support without a bit more work on the underlying coverage analysis tool.

RE your idea to log the results against the Package file, that's worth a try. I wonder if it will syntax highlight the diff or if we want to just print the table in there without coloring (or ditch the diff idea and just print the current coverage table?).

I'd rather have a nicer diff table, anyway, with a single Color.swift -19.34% line instead of the two lines indicating 19.34% was removed and 0% was added -- the current diffing is just a limitation of doing this with what I've got now rather than implementing a new feature on the underlying tool.

To wrap up this message which is more about things I wish for the future than it is about the present, I can play with this more (try out the logging approach) in a few days (or possibly next weekend if I get busy). At the moment I've reached my limit for dealing with how frustrating it can be to troubleshoot random GitHub Action issues :)

@MaxDesiatov
Copy link
Collaborator

No problem at all, I think just logging as is against Package.swift (even the first line) is a good enough approach. Thanks for your hard work!

@mattpolzin
Copy link
Contributor Author

@MaxDesiatov that seems to work; I see the annotation in the Files Changed tab at the bottom.

Would you want to always print the codecov as a warning this way or only log the warning if the coverage was too low. I suppose it could also be a warning normally and an error if the coverage was too low (which is going to fail that CI step anyway)

@MaxDesiatov
Copy link
Collaborator

I suppose it could also be a warning normally and an error if the coverage was too low (which is going to fail that CI step anyway)

Yes, this makes perfect sense. Even if the coverage is improved it's convenient to observe how exactly it changed 👍

@mattpolzin
Copy link
Contributor Author

Well, that seems to be working. On my draft PR linked in this PRs description you can see a failing result whereas this PR passes.

Final question: What do you want the minimum coverage to be initially?

If you like this where it's at, I'll uncomment those tests to get it ready for merging.

@MaxDesiatov
Copy link
Collaborator

As far as I understand, 20% is the current setting, LGTM. Thanks!

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Amazing work! 👏

@MaxDesiatov MaxDesiatov requested a review from a team February 4, 2021 09:40
@j-f1
Copy link
Member

j-f1 commented Feb 4, 2021

This is a OK start, but would it be possible to integrate codecov so people can get a direct link to view more detailed coverage info?

@MaxDesiatov
Copy link
Collaborator

MaxDesiatov commented Feb 4, 2021

We've used codecov.io in the past, before SwiftUI was announced and Tokamak had a React-like API, but I found it to be not very stable overall and hard (if possible at all) to debug if coverage reports somehow don't appear in their UI. As it is a commercial and apparently a VC-funded service, I'm also afraid it will eventually stop serving open-source projects like Travis already did.

Maybe we could upload coverage reports to S3 or GitHub Pages instead? (In some next iteration of this)

@mattpolzin
Copy link
Contributor Author

There might be a way to use GitHub Action artifacts to expose a public URL for the full test coverage table (percent per file).

The action already produces ./codecov.txt. It's easy to upload that file as an artifact, but there won't be a direct link to it by name -- instead, we would need to query the GitHub rest API for artifacts related to the current workflow run and then take the right artifact's ID and plug it into /repos/{owner}/{repo}/actions/artifacts/{artifact_id} ourselves to produce a direct link.

@MaxDesiatov
Copy link
Collaborator

Thanks for taking care of this! I think it's reasonable for more advanced features to be added in separate PRs.

@MaxDesiatov MaxDesiatov merged commit ea94ef9 into TokamakUI:main Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous integration Changes related to the continuous integration process
Development

Successfully merging this pull request may close these issues.

Set up code coverage reports on GitHub Actions
3 participants