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

[MAINT] Change coverage uploader installation source #861

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

LorenzE
Copy link
Member

@LorenzE LorenzE commented Nov 3, 2021

No description provided.

@codecov
Copy link

codecov bot commented Nov 4, 2021

Codecov Report

Merging #861 (f0682dd) into main (ee75517) will increase coverage by 6.98%.
The diff coverage is n/a.

❗ Current head f0682dd differs from pull request most recent head 1df5d35. Consider uploading reports for the commit 1df5d35 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #861      +/-   ##
==========================================
+ Coverage   30.01%   36.99%   +6.98%     
==========================================
  Files         452      196     -256     
  Lines       39269    11584   -27685     
==========================================
- Hits        11786     4286    -7500     
+ Misses      27483     7298   -20185     
Impacted Files Coverage Δ
libraries/fiff/fiff_info.h 0.00% <0.00%> (-100.00%) ⬇️
libraries/fiff/fiff_stream.h 0.00% <0.00%> (-100.00%) ⬇️
libraries/fiff/fiff_info_base.h 0.00% <0.00%> (-100.00%) ⬇️
libraries/fiff/fiff_named_matrix.h 0.00% <0.00%> (-100.00%) ⬇️
libraries/connectivity/connectivitysettings.h 0.00% <0.00%> (-100.00%) ⬇️
libraries/inverse/hpiFit/hpifit.h 0.00% <0.00%> (-50.00%) ⬇️
libraries/connectivity/network/network.h 0.00% <0.00%> (-50.00%) ⬇️
libraries/fiff/fiff_evoked.h 0.00% <0.00%> (-14.29%) ⬇️
libraries/fs/surface.h 0.00% <0.00%> (ø)
libraries/fs/annotation.h 0.00% <0.00%> (ø)
... and 272 more

@juangpc
Copy link
Collaborator

juangpc commented Nov 4, 2021

I really like this @LorenzE , but in an effort for openness, just to let you know, there is a github action wrapping this bash script you're downloading.
Let's see if this is clear:
There is a python based uploader which will be deprecated in feb 1, 2022.
There is a bash uploader which in its version 1 will also be deprecated in feb1, 2022.
But there is another bash uploader (a v2 of the previous one) which is the "thing to use".
There is a github action that uses this bash uploader, although wrapped in javascript with all its glory.

I'm all in to use this. But there is a github action related wrap 👉🏻 👉🏻 https://github.com/codecov/codecov-action

Now that you know about this other option..... I like better the bash script you propose. 😄 It would be great to know the actual coverage locally, is there a way to do that with this bash? have you done ./codecov --help?

@LorenzE
Copy link
Member Author

LorenzE commented Nov 5, 2021

I use the uploader from the official documentation. I guess this is the correct one. I am aware of the github action. The action might be tricky to use in our scenario since we upload the codecov report (via the uploader) after every executed test. codecov is able to merge these individual reports. I am not sure how we can upload/merge reports when using the action. I do not know if codecov can produce reports locally. afaik this is not possible. We would need to use a different reporting tool for that I guess.

@LorenzE LorenzE marked this pull request as draft November 5, 2021 07:43
@juangpc
Copy link
Collaborator

juangpc commented Nov 5, 2021

Yes, that is the correct one, as mentioned in the documentation you link and the one i link too. Both from codecov.

We can change our script to run codecov only once, very easily. If you think this helps, it should be easy. Do let me know.

When you --help the codecov uploader you get a bunch of options. it seems like there is a dryRun version that lets you run codecov but doesn't upload the reports. But as long as i've been able to test it, it doesn't print out the final report or nothing similar.

So I think this helps, but I'm quite dissatisfied with the 'distance' between the testing environment and the developer during it's everyday developing effort / tinkering with the code. You can't make changes, and then wait for minutes (at least) to see their effect in terms of coverage. In this sense, this is not what we need.

But it is clearly something we need, at least to get through the feb 1st, due date.
So thank you.

Let me share what I like:

  • web based.
  • automatic integration into github.
  • I see.
    I like the effort.

@LorenzE
Copy link
Member Author

LorenzE commented Nov 5, 2021

We do not want to let codecov run only once but instead after every test run.

Maybe we can introduce a new tool for local reporting or change to a completely different tool at all. I think codecov has the nicest github integration though.

@juangpc
Copy link
Collaborator

juangpc commented Nov 5, 2021

We do not want to let codecov run only once but instead after every test run.

Why? each test generates its own report file that is later bundled together with whatever tool. Codecov seems to upload each file and do the bundling backend. That would explain why we can't run it locally.

Maybe we can introduce a new tool for local reporting or change to a completely different tool at all. I think codecov has the nicest github integration though.

And web integration too. I also like it. But I wonder what other options we have.

@LorenzE
Copy link
Member Author

LorenzE commented Nov 30, 2021

These changes are not final yet. Please do not merge until this PR looses the draft tag.

@juangpc
Copy link
Collaborator

juangpc commented Dec 2, 2021

Added to #871 .

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

Successfully merging this pull request may close these issues.

2 participants