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.

Sign up for GitHub

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

Jump to bottom

COVERALL: fix broken by changing report from lcov to cobertura #3252

Merged
merged 9 commits into from
May 22, 2024
Merged

COVERALL: fix broken by changing report from lcov to cobertura #3252

merged 9 commits into from
May 22, 2024

Conversation

vncoelho
Copy link
Member

close #3250

@vncoelho
Copy link
Member Author

@neo-project/core , fell free to make any change here if you see that the lastest test is still failing.

My first commit is just to print the report last file.

@vncoelho
Copy link
Member Author

image

Does it run correctly on Draft PRs?
Actions is stuck....

@vncoelho
Copy link
Member Author

image

@vncoelho vncoelho marked this pull request as ready for review May 21, 2024 14:08
@vncoelho vncoelho changed the title COVERALL: fix broken COVERALL: fix broken by changing report from lcov to cobertura May 21, 2024
@vncoelho
Copy link
Member Author

The last data we have is "76.122%",
however, neo-modules had less coverage, thus 74.075% looks good.

@vncoelho
Copy link
Member Author

image

@cschuchardt88, the migration also increased coverage of neo-modules a little

image

@vncoelho
Copy link
Member Author

image

@shargon
Copy link
Member

shargon commented May 21, 2024

Lcov support namespaces I think, is not supported now by Coveralls?

@vncoelho
Copy link
Member Author

Lcov support namespaces I think, is not supported now by Coveralls?

I do know the reason why it stopped working.
It is a simple change and solves for now at least.

@vncoelho
Copy link
Member Author

image

shargon
shargon reviewed May 21, 2024
View reviewed changes
.github/workflows/main.yml Outdated
dotnet test ./tests/Neo.Plugins.Storage.Tests /p:CollectCoverage=true /p:CoverletOutput='${{ github.workspace }}/TestResults/coverage/' /p:MergeWith='${{ github.workspace }}/TestResults/coverage/coverage.json' /p:CoverletOutputFormat='lcov'

dotnet test ./tests/Neo.Plugins.Storage.Tests /p:CollectCoverage=true /p:CoverletOutput='${{ github.workspace }}/TestResults/coverage/' /p:MergeWith='${{ github.workspace }}/TestResults/coverage/coverage.json' /p:CoverletOutputFormat='cobertura'
- name: Coveralls
if: matrix.os == 'ubuntu-latest'
uses: coverallsapp/github-action@v2.2.3
Copy link
Member

@shargon shargon May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Update the version to the with the latest, is not needed two pr for this

Copy link
Member Author

Choose a reason for hiding this comment

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

I will not update it here, @shargon, it can cause other problems. I will wait until it is fixed and the problem is not persistant

Copy link
Member Author

Choose a reason for hiding this comment

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

fell free to approve or open another PR

Copy link
Member Author

Choose a reason for hiding this comment

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

as you said in the other PR, the upgrade in version did not solve the problem, right?

Copy link
Member

@shargon shargon May 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

3 pr for coverals is not required, is one pr per line changed, obviously unnecessary

@vncoelho vncoelho mentioned this pull request May 22, 2024
Merged
12 tasks
@cschuchardt88
Copy link
Member

Lcov support namespaces I think, is not supported now by Coveralls?

I do know the reason why it stopped working. It is a simple change and solves for now at least.

you wait for response from the team that makes the plugin lemurheavy/coveralls-public#1762

@shargon
Copy link
Member

shargon commented May 22, 2024

Lcov support namespaces I think, is not supported now by Coveralls?

I do know the reason why it stopped working. It is a simple change and solves for now at least.

you wait for response from the team that makes the plugin lemurheavy/coveralls-public#1762

It must be a different thing, in dev-pack it works, and I remember that we used lcov because is dotnet prepared, it accept namespaces and assemblies.

Jim8y
Jim8y previously approved these changes May 22, 2024
View reviewed changes
@shargon
Copy link
Member

shargon commented May 22, 2024

@Jim8y the main problem is not the format, in dev pack it works with lcov

@vncoelho
Copy link
Member Author

@shargon ,forget the other PRs and focus on the fix.
The others are another discussion.

@vncoelho
Copy link
Member Author

@Jim8y the main problem is not the format, in dev pack it works with lcov

Try to re-run previous commits of this repo and check results.

@vncoelho
Copy link
Member Author

image

@vncoelho
Copy link
Member Author

I am re-running for improve parse method in neo-cli (#3204) #8447

image

@vncoelho
Copy link
Member Author

image

@vncoelho
Copy link
Member Author

vncoelho commented May 22, 2024 •
edited
Loading

@cschuchardt88, as can be seen, even old commits that were success will now fail.
It should be something with their server or some standard that is present here and is not accepted.

I think this should be merged asap and later moved back to lcov if start to work again.

shargon
shargon reviewed May 22, 2024
View reviewed changes
@shargon shargon mentioned this pull request May 22, 2024
Closed
@shargon shargon merged commit d3cde6d into master May 22, 2024
6 checks passed
@shargon shargon deleted the vncoelho-patch-3 branch May 22, 2024 10:58
@vncoelho
Copy link
Member Author

@shargon
Next time DO NOT CHANGE my pull request without permission, ok?
Commit bbcb52d was masked with a name and you changed version. THIS IS A COMPLETE LACK OF RESPECT FROM YOUR SIDE.

@shargon
Copy link
Member

shargon commented May 22, 2024

@shargon
Next time DO NOT CHANGE my pull request without permission, ok?
Commit bbcb52d was masked with a name and you changed version. THIS IS A COMPLETE LACK OF RESPECT FROM YOUR SIDE.

It's a review. And please, don't blame me for to the same as you did before me #3155 (comment) , and not during review time, during proofs

@vncoelho
Copy link
Member Author

Very good,

That time we were talking on discord and trying to figure out this. Then out of nowhere, you freaked out about changing the draft together.

I will not wash dirty clothes here, @shargon.
I think you should know you are doing.

Jim8y added a commit to Jim8y/neo that referenced this pull request May 25, 2024
@Jim8y
Merge branch 'latest-plugins' of github.com:Jim8y/neo into latest-plu…
4a96d1e 
…gins

* 'latest-plugins' of github.com:Jim8y/neo: (21 commits)
  fix: custom plugins won't shown by command `plugins` (neo-project#3269)
  COVERALL: Improve maintenance and readbility of some variables (neo-project#3248)
  Update nuget (neo-project#3262)
  [**Part-2**] Neo module/master fixes (neo-project#3244)
  Fix `dotnet pack` error (neo-project#3266)
  Fix and Update devcontainer.json to use Dockerfile  (neo-project#3259)
  Add optimization to template (neo-project#3247)
  Optimize plugin's models (neo-project#3246)
  fix CancelTransaction !signers.Any() (neo-project#3263)
  COVERALL: fix broken by changing report from lcov to cobertura (neo-project#3252)
  fix TraverseIterator count (neo-project#3261)
  Native: include DeprecatedIn hardfork into usedHardforks (neo-project#3245)
  [**Part-1**] `neo-module/master` (neo-project#3232)
  Make `ApplicationEngine.LoadContext` protection level `public` (neo-project#3243)
  improve parse method in neo-cli (neo-project#3204)
  Fix neo-project#3239 (neo-project#3242)
  Neo.CLI: enable hardforks for NeoFS mainnet (neo-project#3240)
  v3.7.4 (neo-project#3237)
  fix hardfork issues (neo-project#3234)
  Update src/Neo.CLI/CLI/MainService.Plugins.cs
  ...

# Conflicts:
#	src/Neo.CLI/CLI/MainService.Plugins.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shargon shargon shargon approved these changes

@Jim8y Jim8y Jim8y left review comments

Assignees
No one assigned
Labels
Ready to Merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix coverall
4 participants
@vncoelho @shargon @cschuchardt88 @Jim8y