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

Remove unused Coveralls #1073

Closed
MattiSG opened this issue Oct 26, 2021 · 8 comments · Fixed by #1283
Closed

Remove unused Coveralls #1073

MattiSG opened this issue Oct 26, 2021 · 8 comments · Fixed by #1283
Assignees
Labels
kind:build Pull requests that update a dependency file

Comments

@MattiSG
Copy link
Member

MattiSG commented Oct 26, 2021

Hi there!

I really enjoy OpenFisca, but I recently encountered an issue.

Here is what I did:

  • Submit a PR from a third-party organisation.

Here is what I expected to happen:

  • CI passes.

Here is what actually happened:

  • CI fails on sending data to coveralls.io.

Here is data (or links to it) that can help you reproduce this issue:

#1070 (comment)

Context

I identify more as a:

  • Maintainer.

Coverage data storage was introduced by @maukoquiroga in b785382. It does not seem to be used by anyone in the build / release process, and a quick question on Slack did not yield any other reply. I am in favour of removing coveralls.io altogether, which I suggest to do unless someone chimes in within 4 days to say they use it, with a fast removal if @maukoquiroga confirms it can be removed.

The alternative would be to not execute the Coveralls CI job on third-party PRs.

@MattiSG
Copy link
Member Author

MattiSG commented Oct 26, 2021

The data seems to show that coverage went down from 76% to 54% on 30th of September 2021. Investigating the origin of that change is not obvious.

Capture d’écran 2021-10-26 à 09 13 50

Based on time match, it seems the origin would be #1046, which among other things removed the --capture flag in test_runner.py.

In my opinion, the fact that such a sharp drop in coverage did not trigger any investigation tends to demonstrate this data is not used by anyone. I understand the cost of continuing to store it is low, and it might make sense to keep it since we have data dating back from March 2015, but it would then need to be actually integrated in a quality management agreement.

@MattiSG
Copy link
Member Author

MattiSG commented Oct 26, 2021

Data collection doesn't seem reliable: #1057 (comment)

@bonjourmauko
Copy link
Member

Hello @MattiSG , I do personally do not care about the history, but I do care about coverage.

In my opinion, the fact that such a sharp drop in coverage did not trigger any investigation tends to demonstrate this data is not used by anyone.

I do, but coverage is rather useless as a socio-technical mechanism unless there is a policy enforced programatically.

Concerning data collection, I proposed in #1059 what I think is the way it should be done.

Concerning Coveralls itself, as a service, we can remove it 😃

@bonjourmauko
Copy link
Member

Just to complement my previous comment:

  1. We can remove the service Coveralls
  2. I'll propose a PR to fix the coverage issue —thanks for reporting it!

@bonjourmauko
Copy link
Member

Thanks to this issue, I discovered that doctests are not being run 🤯

This was referenced Oct 26, 2021
@bonjourmauko
Copy link
Member

This issue has again come to my attention now that we have matrix builds. Under that new scenario, I see no usefulness of collecting coverage information with Coveralls.

@benjello
Copy link
Member

Please go on and remove the service no one use.

@bonjourmauko
Copy link
Member

It is a low hanging fruit, @benjello don't you want to propose a PR?

@bonjourmauko bonjourmauko added the kind:build Pull requests that update a dependency file label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:build Pull requests that update a dependency file
Projects
Development

Successfully merging a pull request may close this issue.

3 participants