Adds test coverage for non generated features #48
Conversation
nickfloyd
added
the
Type: Maintenance
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with |
.github/workflows/build.yml
Outdated
| run: dotnet test --configuration Release --verbosity normal --collect:"XPlat Code Coverage" --results-directory ./coverage -p:ExcludeByFile="**/GitHub/**/*.cs" | ||
|
|
||
| - name: Code Coverage Report | ||
| uses: irongut/CodeCoverageSummary@v1.3.0 |
There was a problem hiding this comment.
Do you mind explaining why you've gone with this tool? I haven't heard of it before and I'm curious what the alternatives and tradeoffs were.
There was a problem hiding this comment.
Sure... it was the most streamlined, most starred, I would've liked it to be more recently maintained - some other implementations had gists, repos, and accounts tied to it so while I was evaluating different tooling and trying to decide if we even want to do something like this - it seemed to be the most reasonable option.
.github/workflows/build.yml
Outdated
| thresholds: '60 80' | ||
|
|
||
| - name: Add Coverage PR Comment | ||
| uses: marocchino/sticky-pull-request-comment@v2 |
There was a problem hiding this comment.
I'm also curious about this choice, since many other options exist here, like https://github.com/thollander/actions-comment-pull-request.
There was a problem hiding this comment.
As with the above - this was the favored implementation and streamlined option. I'm not sure I am 100% sold on having a PR comment with coverage details - this is really try on the shoe and see if it fits. I am most likely going to pull this out and leave it as reporting in the CI step and leave bubbling up the results for another PR.
test/Client/ClientFactoryTest.cs
Outdated
| Assert.Contains(handlers, h => h is UserAgentHandler); | ||
| } | ||
|
|
||
| // ChainHandlersCollectionAndGetFirstLink |
There was a problem hiding this comment.
What's the purpose of this comment and the below one on line 58? Are they going to be expanded upon?
There was a problem hiding this comment.
Yeah this needs to be cleaned up - I began grouping tests and this was just a remark to keep things ordered for me.
|
|
Before the change?
After the change?
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!