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

Fix coverage #3155

Merged
merged 23 commits into from
Feb 23, 2024
Merged

Fix coverage #3155

merged 23 commits into from
Feb 23, 2024

Conversation

shargon
Copy link
Member

@shargon shargon commented Feb 22, 2024

The coverage is sometimes random, this pr aims to fix it

@vncoelho
Copy link
Member

this will be good if fixed

@vncoelho
Copy link
Member

Option path-to-lcov is now deprecated, use file instead.

@vncoelho
Copy link
Member

maybe version v2.2.3

.github/workflows/main.yml Outdated Show resolved Hide resolved
@vncoelho
Copy link
Member

vncoelho commented Feb 22, 2024

"You can also skip using the file option and coveralls will try to find all supported coverage files and combine their data."
"If coveralls fails to determine your coverage report format, use explicit format option to specify it. See supported values."

@shargon
Copy link
Member Author

shargon commented Feb 22, 2024

I think that now it's fixed!

@vncoelho vncoelho marked this pull request as ready for review February 22, 2024 23:31
vncoelho
vncoelho previously approved these changes Feb 22, 2024
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I think it is fixed, it was good to check their release info.

Maybe we can also leave the file config empty and delete it.

@shargon
Copy link
Member Author

shargon commented Feb 22, 2024

It only take cryptography https://coveralls.io/builds/65868879

.github/workflows/main.yml Outdated Show resolved Hide resolved
@vncoelho
Copy link
Member

I just tried something to be sure, let's see the result.

Recently I got some problems on devcontainer if these commands were not executed before

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@vncoelho
Copy link
Member

Since the last command is just used for ubuntu

maybe remove the rest of the parameter for the other matrix.os

    - name: Test
      run: |
        find tests -name *.csproj | xargs -I % dotnet add % package coverlet.msbuild
        dotnet test /p:CollectCoverage=true /p:CoverletOutputFormat=lcov /p:CoverletOutput=${GITHUB_WORKSPACE}/coverage/lcov /p:MergeWith="./coverage/lcov.net7.0.info"
    - name: Coveralls
      if: matrix.os == 'ubuntu-latest'
      uses: coverallsapp/github-action@v2.2.3
      with:
        github-token: ${{ secrets.GITHUB_TOKEN }}
        file: ./coverage/lcov.net7.0.info
        ```

.github/workflows/main.yml Outdated Show resolved Hide resolved
@vncoelho
Copy link
Member

vncoelho commented Feb 22, 2024

why we need merge? Which additional reports should be added?

@shargon
Copy link
Member Author

shargon commented Feb 22, 2024

Please @vncoelho wait for my wait for review
Screenshot_20240223_005512_Chrome

@shargon
Copy link
Member Author

shargon commented Feb 23, 2024

The doc said that it's required to merge the test one by one, and convert to the desired format in the last one:

https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/Examples/MSBuild/MergeWith/HowTo.md

.github/workflows/main.yml Outdated Show resolved Hide resolved
@vncoelho
Copy link
Member

The doc said that it's required to merge the test one by one, and convert to the desired format in the last one:

https://github.com/coverlet-coverage/coverlet/blob/master/Documentation/Examples/MSBuild/MergeWith/HowTo.md

I saw this as well later

@vncoelho
Copy link
Member

I think you should use the default as well without specifying, it will be json

/p:CoverletOutput=../CoverageResults/ /p:MergeWith="../CoverageResults/coverage.json"

.github/workflows/main.yml Outdated Show resolved Hide resolved
.github/workflows/main.yml Outdated Show resolved Hide resolved
@shargon shargon marked this pull request as ready for review February 23, 2024 00:37
@shargon
Copy link
Member Author

shargon commented Feb 23, 2024

It works now :)

@shargon shargon requested a review from Jim8y February 23, 2024 00:48
@shargon shargon merged commit de39a36 into master Feb 23, 2024
7 checks passed
@shargon shargon deleted the core-fix-coverarals branch February 23, 2024 00:50
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

perfect, I think you can change to

    - name: Test for Win and Mac
      if: matrix.os == 'windows-latest' || matrix.os == 'macos-latest'   
      run: |
        dotnet test     
    - name: Test for Coveralls Only
      if: matrix.os == 'ubuntu-latest'  

@shargon
Copy link
Member Author

shargon commented Feb 23, 2024

This should also be done in modules and devpack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants