Re-add support for Coverage reports through codecov.io

[dev-zero]

No description provided.

[CasperWA]

Looks good, thanks @dev-zero!

I have made a suggested change via a PR to your branch to only run the actions on push to the master and develop branches, as well as always for PRs.

[CasperWA]

Info: I added the secret CODECOV_TOKEN.

[CasperWA]

Good for me, and mergeable when tests succeed.

We're discussing whether this is the right moment to merge this (also this would only show a 9% coverage from the pytests only

[CasperWA]

@dev-zero will you create an issue in codecov/codecov-action for the failure or maybe reopen codecov/codecov-action#22 ?

[CasperWA]

It seems we need to update to codecov-action v1.0.5 according to this issue.

[dev-zero]

@CasperWA https://community.codecov.io/t/whitelist-github-action-servers-to-upload-without-a-token/491/5 ... Travis doesn't need a token because Codecov/Coveralls can verify the request via an API, but for GHA there's no API yet

[dev-zero]

Short summary: Coverage through codecov or coveralls is not possible until GitHub releases an API to query GitHub Actions such that codecov/coveralls can verify the uploader. Supposed to happen in Q1 2020. The secret token is for obvious reasons not available in a PR, only in builds initiated by direct commits on the repo (currently restricted to develop and master).

We have two options for now: disable coverage upload for pull requests or not fail the CI when uploading to coverage report fails.

[ltalirz]

We have two options for now: disable coverage upload for pull requests or not fail the CI when uploading to coverage report fails.

I think it doesn't really matter.
If you go for the second option, then things might start to "magically" work at some point, so perhaps that's an argument for the second option, if there is an easy way to implement it.

[codecov-io]

Codecov Report

Merging #3618 into develop will decrease coverage by 1.67%.
The diff coverage is 73.76%.

@@            Coverage Diff             @@
##           develop   #3618      +/-   ##
==========================================
- Coverage    70.87%   69.2%   -1.68%     
==========================================
  Files          458     458              
  Lines        33545   33766     +221     
==========================================
- Hits         23775   23367     -408     
- Misses        9770   10399     +629

Flag	Coverage Δ
#django	69.2% <73.76%> (+3.79%)	⬆️
#sqlalchemy	?

Impacted Files	Coverage Δ
aiida/orm/nodes/process/workflow/workchain.py	91.66% <ø> (-0.65%)	⬇️
aiida/common/datastructures.py	96.55% <ø> (ø)	⬆️
aiida/backends/djsite/manager.py	79.31% <ø> (ø)	⬆️
aiida/backends/sqlalchemy/manager.py	0% <ø> (-76%)	⬇️
aiida/backends/djsite/db/models.py	77.19% <ø> (ø)	⬆️
aiida/engine/processes/workchains/context.py	50% <ø> (ø)	⬆️
aiida/orm/nodes/process/process.py	85.2% <ø> (-0.09%)	⬇️
aiida/cmdline/utils/multi_line_input.py	80.48% <0%> (ø)	⬆️
aiida/manage/configuration/setup.py	71.42% <100%> (ø)	⬆️
aiida/backends/djsite/db/migrations/__init__.py	68.83% <100%> (+68.83%)	⬆️
... and 138 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb8fc99...54049f7. Read the comment docs.

[CasperWA]

Testing this in the branch feature/codecov-integration-PR-3618, this is the uploaded coverage report.

As seen in the codecov bot comment here, the total coverage is 70.87 % - it doesn't seem to catch a lot of the database migration files/tests.

Furthermore, anything that isn't in the realm of pytest, i.e. .ci/test_daemon.py, as well as .ci/test_plugin_testcase.py, will not be covered (I believe).

[ltalirz]

Yeah, 0% coverage for both the the django migrations and the sqlalchemy migrations. I wonder why...

Furthermore, anything that isn't in the realm of pytest, i.e. .ci/test_daemon.py, as well as .ci/test_plugin_testcase.py, will not be covered (I believe).

Right, there one should be able to run it with something like coverage run ...

[ltalirz]

It would be great if we could move forward with this.
Does anyone have an idea why coverage would suddenly stop noticing migration-related tests?

[giovannipizzi]

I think it's just because we are asking coverage to omit the whole folder (together with some more) in the .coveragerc:

aiida-core/.coveragerc

Line 3 in 557a6a8

omit = aiida/test*.py,aiida/*/test*.py,aiida/*/*/test*.py,aiida/*/*/*/test*.py,aiida/*/*/*/*/test*.py,aiida/*/*/*/*/*/test*.py,aiida/*/migrations/*.py,aiida/*/migrations/versions/*.py

I'm not sure anymore why this was introduced in the first place (in any case, it was introduced when migrations were untested).

Maybe we don't need to omit anything? You might want to just push a commit removing the omit line and check how things change.

[ltalirz]

Great, thanks a lot - will look into this now

[ltalirz]

Removing the omissions specified in .coveragerc brings back coverage on migrations.
Test coverage now reported at 77%.

To me, this PR is good to merge - does anybody still want to give another look?
One question to me is whether we really need the coverage output in the travis logs (it's about 500 lines)...
Anyhow, perhaps it's useful to keep at least until codecov starts working on PRs.
Another point is that we could try and remove the .coveragerc file entirely.

Finally, Casper is right that there are still two tests that don't report coverage:
.ci/test_daemon.py, as well as .ci/test_plugin_testcase.py

I'll have a look into moving the daemon test to pytest (in a separate PR) - that should not be a big deal.
The plugin testcase is explicitly testing a unittest test runner, and so there is not much point in moving it - we could still run it via coverage run ... but then we need to make sure to enable the same options. Doable as well, but not very important.

[CasperWA]

Removing the omissions specified in .coveragerc brings back coverage on migrations.
Test coverage now reported at 77%.

Good catch on this @giovannipizzi and @ltalirz !

Finally, Casper is right that there are still two tests that don't report coverage:
.ci/test_daemon.py, as well as .ci/test_plugin_testcase.py

I'll have a look into moving the daemon test to pytest (in a separate PR) - that should not be a big deal.
The plugin testcase is explicitly testing a unittest test runner, and so there is not much point in moving it - we could still run it via coverage run ... but then we need to make sure to enable the same options. Doable as well, but not very important.

If it's easily done, it wouldn't hurt? But if it doesn't change the coverage, there is no reason to do this.

[sphuber]

If it's easily done, it wouldn't hurt? But if it doesn't change the coverage, there is no reason to do this.

It should increase the coverage because for example the BaseRestartWorkChain class is only tested in test_daemon.py. This goes for quite some other classes like ArithmeticAddCalculation and TemplatereplacerCalculation although they are less critical since they mostly exist for testing and demonstration purposes.

[CasperWA]

If it's easily done, it wouldn't hurt? But if it doesn't change the coverage, there is no reason to do this.

It should increase the coverage because for example the BaseRestartWorkChain class is only tested in test_daemon.py. This goes for quite some other classes like ArithmeticAddCalculation and TemplatereplacerCalculation although they are less critical since they mostly exist for testing and demonstration purposes.

All right - I'll look into doing this - unless anyone else is already on top of it? (@ltalirz, @dev-zero ...)

[ltalirz]

@sphuber Not sure whether what I wrote was clear - I meant I will move the test_daemon to pytest (since this one matters), while the test_plugin_testcase can also be run via coverage (but is not very important).

@CasperWA If you prefer to do it instead, go ahead!
I'd still suggest to do it in a separate PR.

[CasperWA]

@CasperWA If you prefer to do it instead, go ahead!
I'd still suggest to do it in a separate PR.

Ah right! Yeah, sure, okay. I've already started some work on it now, so I'll continue, but can surely make a new PR for it. I'll just squash our contributions into 3 separate commits and then you're free to re-approve and merge.

[ltalirz]

thanks @CasperWA !

[CasperWA]

To note: Since codecov still does not support token-less uploads of coverage reports via GitHub Actions CI, the coverage will only be updated for a PR when it is merged (unless it's coming from an aiidateam branch already).

[ltalirz]

Yes, that's ok. I let you merge - not sure whether you preferred not to squash here...

[CasperWA]

Yes, that's ok. I let you merge - not sure whether you preferred not to squash here...

I already squashed (what I intended to squash), but for multiple contributions we usually keep a commit per contributor, right? (Calling @sphuber).

[sphuber]

Yes, that's ok. I let you merge - not sure whether you preferred not to squash here...

I already squashed, but for multiple contributions we usually keep a commit per contributor, right? (Calling @sphuber).

If the contributions are significant, yes, I typically keep them. Sometimes, if an additional commit is really small though, one can opt to use Co-Authored-By: John Doe <email@address> for the contributors at the end of the commit message of the squashed commit.

Edit: to clarify, if you deem these commits worthwhile as being separate, by all means keep them and just rebase-merge 👍 fire away

[CasperWA]

I went through them, and they all add the right amount of sugar - so they all stay 😃