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 Coveralls support / Coveralls setup broken #1463

Open
buhtz opened this issue Jun 28, 2023 · 8 comments
Open

Remove Coveralls support / Coveralls setup broken #1463

buhtz opened this issue Jun 28, 2023 · 8 comments
Assignees
Labels
Feedback needs user response, may be closed after timeout without a response Infrastructure Low relevant, but not urgent

Comments

@buhtz
Copy link
Member

buhtz commented Jun 28, 2023

I'm sorry I have to open this as an Issue because I don't know whats going on here. It seems that the setup on https://coveralls.io/github/bit-team/backintime is not correct. I do have admin rights (based on my bit-team membership). But I don't understand that UI. So I hope that Germar, Aryoda or someone else can help out here.

The badge on README.md do point to master which should be dev of course. Easy to fix. But I'm not sure if or how coveralls can check for "dev" instead of "master".

I'm not sure how coveralls get's its data but I know there are some coveralls-calls in our TravisCI build setup. Not sure if this is bound to a specific branch.

The landing page looks like this to me and confuses me a lot

image

In the top right you see "dev" with 50% and also "dev" as default branch. But in the bottom there is another branch.

The branch-dropdown-menu do not offer me "dev" even after clicking on "sync branches".

I checked the settings but before I break something I thought it is better to ask.

Related to #1282

@buhtz buhtz added Infrastructure Low relevant, but not urgent labels Jun 28, 2023
@buhtz buhtz added HELP-WANTED Used by 24pullrequests.com to suggest issues GOOD FIRST ISSUE Used by 24pullrequests.com to suggest issues labels Aug 9, 2023
@buhtz buhtz added this to the 3rd release from now milestone Aug 23, 2023
@buhtz buhtz changed the title Coveralls setup Remove Coveralls support / Coveralls setup broken Aug 30, 2023
@buhtz
Copy link
Member Author

buhtz commented Aug 30, 2023

I vote to remove Coveralls service and account because I see no value in it and there is no one who want to take care of it.

Coverage as a measurement has its values but could be done locally on the machine of each developer.

The maintenance cost is high. In regards to #1489 I investigated and modified the make system. I discovered that there is some exceptional code handling coveralls on travisci. Without coveralls the travisci exceptions could be removed and the code is easier to maintain. Maybe there is away to remove the travisci-exception code from configure-make but keeping coveralls. But for that we need someone who understand coveralls and travisci configuration.

@emtiu
Copy link
Member

emtiu commented Aug 30, 2023

If it's unmaintained and causes complexity, then I agree with removing it.

@buhtz buhtz self-assigned this Sep 12, 2023
@buhtz
Copy link
Member Author

buhtz commented Oct 2, 2023

It seems that we have admin rights to the coveralls project. So we can delete it our self. But I'll contact Germar first via mail.

@buhtz buhtz added the Feedback needs user response, may be closed after timeout without a response label Oct 26, 2023
@Germar
Copy link
Member

Germar commented Nov 8, 2023

The badge on README.md do point to master wich should be dev of course. Easy to fix. But I'm not sure if or how coveralls can check for "dev" instead of "master".

Just change like this will fix the badge

[![Coverage Status](https://coveralls.io/repos/github/bit-team/backintime/badge.svg?branch=dev)](https://coveralls.io/github/bit-team/backintime?branch=dev)

Like I said in #1553 (comment)_ I used coveralls.io to monitor the impact of tests. For me coveralls.io always worked flawlessly in background. These three lines do the trick:

backintime/.travis.yml

Lines 50 to 52 in 6df8c90

after_success:
- coverage combine
- coveralls

@buhtz
Copy link
Member Author

buhtz commented Dec 6, 2023

I am still not convinced and vote to remove coveralls support.

To "monitor the impact of tests" someone can clone the repo and run coveralls on their local machine. I see no value in doing this in public.

In my understanding based on my experience and studying of that topic the coverage of productive code by testing code only counts for unit tests. Integration and system tests do not count because they do not test one isolated behavior (sometimes misinterpreted as a block of code). They always test multiple behaviors at once.

Currently there is no pure unit test in BITs test suite. Because of my previous argument the coverage value is misleading. The code is nearly uncovered.

One intention of unit tests is to make it secure to do modifications in the code. I need to be sure that the modification of one line won't affect other parts of the code and its behavior. In the current situation I can not be sure.

@aryoda
Copy link
Contributor

aryoda commented Dec 7, 2023

To "monitor the impact of tests" someone can clone the repo and run coveralls on their local machine. I see no value in doing this in public.

I think coveralls is kinda quality indicator for our users since they

  • can see the test coverage (and yes, this may be misleading or non-comparable to other Apps but hey - this is marketing ;-)
  • can see how good the developers care about testing by looking into the coverage history
    (if the percentage of covered lines of code drops over time this may be a bad sign for the app quality)

I also don't expect much maintenance overhead since coveralls is just started by executing the unit tests (some text execution exclusions may be required for Travis CI - not coveralls per se.

So I would like to keep coveralls because it doesn't hurt and I do not always run the code coverage check locally...

@buhtz
Copy link
Member Author

buhtz commented Dec 8, 2023

I think coveralls is kinda quality indicator for our users since they

I am aware of the marketing aspect and also like it. But the cost is higher then the benefit.

So I would like to keep coveralls because it doesn't hurt and I do not always run the code coverage check locally...

In the current state it hurts because the number is incorrect even if we count our tests as real unit tests. I am not sure but it seems to me that the coverage calculation do take the unit test code and its coverage also into account.

And we need someone to maintain it. Currently there is no one. The coveralls.io is stupid. Example:
grafik

Top right "dev" branch is selected. But the list shows a pull request that belong to another branch.

And "0 of 0 relevant lines covered". WTF!?

I can not find a list of files and their values.

It is unclear/nontransparent where the current "75%" comes from. I assume that this value comes from a very old build long ago in the past.

The UI is not only bad it is broken. And I don't want to invest more time into it.

EDIT: I triggered a manual build of the dev branch on TravisCI. Now some file information appear on coveralls.io.
image

As you can see here. The test files are included in the calculation and the common folder is not even included.

EDIT2: Even on my local machine it is not possible to generate an evident coverage report because of our project setup. See #1575 .

Partly "code coverage" (not test coverage) just in common:

Name                                                                         Stmts   Miss  Cover
------------------------------------------------------------------------------------------------
/home/user/mluCloud/my.work/bit/backintime/qt/plugins/notifyplugin.py           35     22    37%
/home/user/mluCloud/my.work/bit/backintime/qt/plugins/systrayiconplugin.py      44     19    57%
applicationinstance.py                                                          79      4    95%
backintime.py                                                                  534    258    52%
bcolors.py                                                                      18      8    56%
cli.py                                                                         171    153    11%
config.py                                                                      952    428    55%
configfile.py                                                                  330     18    95%
diagnostics.py                                                                 146     22    85%
encfstools.py                                                                  421    358    15%
exceptions.py                                                                   38      8    79%
languages.py                                                                     2      0   100%
logger.py                                                                       65      5    92%
mount.py                                                                       340     88    74%
password.py                                                                    198    156    21%
password_ipc.py                                                                 82     58    29%
pluginmanager.py                                                               134     35    74%
plugins/usercallbackplugin.py                                                   56     32    43%
progress.py                                                                     16      2    88%
snapshotlog.py                                                                  89     12    87%
snapshots.py                                                                  1339    313    77%
sshtools.py                                                                    423     90    79%
tools.py                                                                      1143    396    65%
------------------------------------------------------------------------------------------------
TOTAL                                                                         6655   2485    63%

Be aware that some *.py files with productive code (askpass.py, driveinfo.py, guiapplicationinstance.py, ...) are missing in this calculation because they are never loaded by pytest/unittest. This problem will disappear when fixing #1575. I assume that even if we would have a coveralls.io expert it is not possible to setup the report in a realistic way without hacky workarounds until we migrate the projects infrastructure to a Python build-system.

Interpretation of the result: The value "63%" is optimistic because some files with productive code are missing. So make it 55% I would say. But just for common. Put qt onto it wich is nearly uncovered the value do dramatically decrease again. My broad but optimistic estimation would be 20 to 30%. And again: This is just code coverage and not test coverage because we do not have isolated unit test.
Do not take this wrong. Integration and system tests are needed and valuable also. But there goal is different. They do not solve the problem with introducing bugs and modified behavior via fixing other bugs. It is just luck (or a side effect) when they catch new bugs.

Using coveralls.io is IMHO not an indicator of quality in our case. It is the opposite.

EDIT3: Just for your information here an Issue-based discussion about why coverage.py by default do measure by more then just the productive code. nedbat/coveragepy#1707

EDIT4: As more I get into this topic (e.g. my tec demo) and the unexpected behavior of coverage.py I come to the conclusion that coverage values of most of the projects out there are just wrong because coverage.py is not correct configured and its default behavior is useless in context of code quality and test coverage.

@buhtz buhtz removed GOOD FIRST ISSUE Used by 24pullrequests.com to suggest issues HELP-WANTED Used by 24pullrequests.com to suggest issues labels Jan 16, 2024
@buhtz
Copy link
Member Author

buhtz commented May 17, 2024

There have been no new arguments on this matter since it was last discussed here. Since this blocks the upcoming release, the active maintainer team has made a decision to remove coveralls from the current project. After migration to modern python packaging standards (#1575) coveragepy will be configured again to calculate strict test coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feedback needs user response, may be closed after timeout without a response Infrastructure Low relevant, but not urgent
Projects
None yet
Development

No branches or pull requests

4 participants