Re: Gitlab CI: failed unit tests vs. currently passing CI
On Mon, Jan 24, 2022 at 12:56 AM Albert Astals Cid wrote: > El diumenge, 23 de gener de 2022, a les 1:59:01 (CET), Ben Cooksley va > escriure: > > On Sun, Jan 23, 2022 at 12:29 PM Albert Astals Cid > wrote: > > > > > El diumenge, 23 de gener de 2022, a les 0:09:23 (CET), Ben Cooksley va > > > escriure: > > > > On Sun, Jan 23, 2022 at 11:29 AM Albert Astals Cid > > > wrote: > > > > > > > > > El dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben > Cooksley va > > > > > escriure: > > > > > > EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM > > > Friedrich > > > > > > W. H. Kossebau wrote: > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > seems that Gitlab CI is currently configured to show the green > > > > > "Success" > > > > > > > checkmark for pipeline runs even if unit tests are failing. > > > > > > > > > > > > > > > > > > > That is correct, only compilation or other internal failures > cause > > > the > > > > > > build to show a failure result. > > > > > > > > > > > > > > > > > > > Reasons seems to be that there Gitlab only knows Yay or Nay, > > > without > > > > > the > > > > > > > warning state level known from Jenkins. > > > > > > > > > > > > > > > > > > > Also correct. > > > > > > > > > > > > > > > > > > > And given that quite some projects (sadly) maintain a few > long-time > > > > > > > failing > > > > > > > unit tests, having the pipeline fail on unit tests seems to > have > > > been > > > > > seen > > > > > > > as > > > > > > > too aggressive > > > > > > > > > > > > > > > > > > Correct again. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > This of course harms the purpose of the unit tests, when their > > > failures > > > > > > > are > > > > > > > only noticed weeks later, not e.g. at MR discussion time. > > > > > > > > > > > > > > > > > > > Gitlab does note changes in the test suite as can currently be > seen > > > on > > > > > > https://invent.kde.org/frameworks/kio/-/merge_requests/708 > > > > > > Quoting the page: "Test summary contained 33 failed and 16 fixed > > > test > > > > > > results out of 205 total tests" > > > > > > > > > > > > It does the same thing for Code Quality - "Code quality scanning > > > detected > > > > > > 51 changes in merged results" > > > > > > > > > > Don't want to derail the confirmation, but those results are > terrible, > > > > > they always say things changed in places not touched by the code of > > > the MR, > > > > > any idea why? > > > > > > > > > > > > > Unfortunately not - my only guess would be that cppcheck reports > results > > > > slightly differently which Gitlab has issues interpreting. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Seeing how at least in KDE Frameworks first regressions > sneaked in > > > > > without > > > > > > > someone noticing (nobody looks at logs when the surface shows a > > > green > > > > > > > checkmark, e.g. kcoreaddons, kwidgetsaddons, kio, purpose, > krunner > > > on > > > > > > > openSUSE > > > > > > > and possibly more have regressed in recent weeks, see > > > build.kde.org) > > > > > this > > > > > > > should be something to deal with better, right? > > > > > > > > > > > > > > > > > > > Bhushan gave two first ideas just now on #kde-sysadmin: > > > > > > > > Well we can add a switch that repos can commit to saying test > > > > > failure is > > > > > > > build failure > > > > > > > > Another alternative is we use bot to write a comment on MR > > > > > > > > > > > > > > IMHO, to give unit tests the purpose they have, we should by > > > default to > > > > > > > let > > > > > > > test failures be build failures. And have projects opt out if > they > > > > > need to > > > > > > > have some unit tests keep failing, instead of e.g. tagging them > > > with > > > > > > > expected > > > > > > > failures or handling any special environment they run into on > the > > > CI. > > > > > > > > > > > > > > Your opinions? > > > > > > > > > > > > > > > > > > > The switch will need to be around the other way i'm afraid as > there > > > are > > > > > > simply too many projects with broken tests right now. > > > > > > The best place for that switch will be in .kde-ci.yml. > > > > > > > > > > > > My only concern however would be abuse of this switch, much in > the > > > way > > > > > that > > > > > > certain projects abuse EXCLUDE_DEPRECATED_BEFORE_AND_AT. > > > > > > The last thing we would want would be for people to flip this > switch > > > and > > > > > > then leave their CI builds in a failing state - meaning that > actual > > > > > > compilation failures would be missed (and then lead to CI > maintenance > > > > > > issues) > > > > > > > > > > > > Thoughts on that? > > > > > > > > > > Test failing should mark the CI as failed, anything other than that > > > > > doesn't make sense. The CI did fail marking it as passed is lying > to > > > > > ourselves. > > > > > > > > > > > > > We can *still* merge failed MR with failed CI, the Merge button is > ju
Re: Gitlab CI: failed unit tests vs. currently passing CI
El diumenge, 23 de gener de 2022, a les 1:59:01 (CET), Ben Cooksley va escriure: > On Sun, Jan 23, 2022 at 12:29 PM Albert Astals Cid wrote: > > > El diumenge, 23 de gener de 2022, a les 0:09:23 (CET), Ben Cooksley va > > escriure: > > > On Sun, Jan 23, 2022 at 11:29 AM Albert Astals Cid > > wrote: > > > > > > > El dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben Cooksley va > > > > escriure: > > > > > EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM > > Friedrich > > > > > W. H. Kossebau wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > seems that Gitlab CI is currently configured to show the green > > > > "Success" > > > > > > checkmark for pipeline runs even if unit tests are failing. > > > > > > > > > > > > > > > > That is correct, only compilation or other internal failures cause > > the > > > > > build to show a failure result. > > > > > > > > > > > > > > > > Reasons seems to be that there Gitlab only knows Yay or Nay, > > without > > > > the > > > > > > warning state level known from Jenkins. > > > > > > > > > > > > > > > > Also correct. > > > > > > > > > > > > > > > > And given that quite some projects (sadly) maintain a few long-time > > > > > > failing > > > > > > unit tests, having the pipeline fail on unit tests seems to have > > been > > > > seen > > > > > > as > > > > > > too aggressive > > > > > > > > > > > > > > > Correct again. > > > > > > > > > > > > > > > > > > > > > > > > > > > > This of course harms the purpose of the unit tests, when their > > failures > > > > > > are > > > > > > only noticed weeks later, not e.g. at MR discussion time. > > > > > > > > > > > > > > > > Gitlab does note changes in the test suite as can currently be seen > > on > > > > > https://invent.kde.org/frameworks/kio/-/merge_requests/708 > > > > > Quoting the page: "Test summary contained 33 failed and 16 fixed > > test > > > > > results out of 205 total tests" > > > > > > > > > > It does the same thing for Code Quality - "Code quality scanning > > detected > > > > > 51 changes in merged results" > > > > > > > > Don't want to derail the confirmation, but those results are terrible, > > > > they always say things changed in places not touched by the code of > > the MR, > > > > any idea why? > > > > > > > > > > Unfortunately not - my only guess would be that cppcheck reports results > > > slightly differently which Gitlab has issues interpreting. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Seeing how at least in KDE Frameworks first regressions sneaked in > > > > without > > > > > > someone noticing (nobody looks at logs when the surface shows a > > green > > > > > > checkmark, e.g. kcoreaddons, kwidgetsaddons, kio, purpose, krunner > > on > > > > > > openSUSE > > > > > > and possibly more have regressed in recent weeks, see > > build.kde.org) > > > > this > > > > > > should be something to deal with better, right? > > > > > > > > > > > > > > > > Bhushan gave two first ideas just now on #kde-sysadmin: > > > > > > > Well we can add a switch that repos can commit to saying test > > > > failure is > > > > > > build failure > > > > > > > Another alternative is we use bot to write a comment on MR > > > > > > > > > > > > IMHO, to give unit tests the purpose they have, we should by > > default to > > > > > > let > > > > > > test failures be build failures. And have projects opt out if they > > > > need to > > > > > > have some unit tests keep failing, instead of e.g. tagging them > > with > > > > > > expected > > > > > > failures or handling any special environment they run into on the > > CI. > > > > > > > > > > > > Your opinions? > > > > > > > > > > > > > > > > The switch will need to be around the other way i'm afraid as there > > are > > > > > simply too many projects with broken tests right now. > > > > > The best place for that switch will be in .kde-ci.yml. > > > > > > > > > > My only concern however would be abuse of this switch, much in the > > way > > > > that > > > > > certain projects abuse EXCLUDE_DEPRECATED_BEFORE_AND_AT. > > > > > The last thing we would want would be for people to flip this switch > > and > > > > > then leave their CI builds in a failing state - meaning that actual > > > > > compilation failures would be missed (and then lead to CI maintenance > > > > > issues) > > > > > > > > > > Thoughts on that? > > > > > > > > Test failing should mark the CI as failed, anything other than that > > > > doesn't make sense. The CI did fail marking it as passed is lying to > > > > ourselves. > > > > > > > > > > We can *still* merge failed MR with failed CI, the Merge button is just > > > > red, but it will work. > > > > > > > > > > There is a big difference between "this doesn't compile" (because someone > > > forgot to commit a header/etc, dependency change that isn't in place or > > > because of a platform specific issue) and "some tests failed". > > > What that encourages is for people to ignore the results from the CI > > system > > > - as t
Re: Gitlab CI: failed unit tests vs. currently passing CI
On Sun, Jan 23, 2022 at 12:38 PM Albert Astals Cid wrote: > El diumenge, 23 de gener de 2022, a les 0:09:23 (CET), Ben Cooksley va > escriure: > > On Sun, Jan 23, 2022 at 11:29 AM Albert Astals Cid > wrote: > > > > > El dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben Cooksley va > > > escriure: > > > > EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM > Friedrich > > > > W. H. Kossebau wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > seems that Gitlab CI is currently configured to show the green > > > "Success" > > > > > checkmark for pipeline runs even if unit tests are failing. > > > > > > > > > > > > > That is correct, only compilation or other internal failures cause > the > > > > build to show a failure result. > > > > > > > > > > > > > Reasons seems to be that there Gitlab only knows Yay or Nay, > without > > > the > > > > > warning state level known from Jenkins. > > > > > > > > > > > > > Also correct. > > > > > > > > > > > > > And given that quite some projects (sadly) maintain a few long-time > > > > > failing > > > > > unit tests, having the pipeline fail on unit tests seems to have > been > > > seen > > > > > as > > > > > too aggressive > > > > > > > > > > > > Correct again. > > > > > > > > > > > > > > > > > > > > > > > This of course harms the purpose of the unit tests, when their > failures > > > > > are > > > > > only noticed weeks later, not e.g. at MR discussion time. > > > > > > > > > > > > > Gitlab does note changes in the test suite as can currently be seen > on > > > > https://invent.kde.org/frameworks/kio/-/merge_requests/708 > > > > Quoting the page: "Test summary contained 33 failed and 16 fixed > test > > > > results out of 205 total tests" > > > > > > > > It does the same thing for Code Quality - "Code quality scanning > detected > > > > 51 changes in merged results" > > > > > > Don't want to derail the confirmation, but those results are terrible, > > > they always say things changed in places not touched by the code of > the MR, > > > any idea why? > > > > > > > Unfortunately not - my only guess would be that cppcheck reports results > > slightly differently which Gitlab has issues interpreting. > > Can we just disable it? > Various things can be configured on a per-project basis. cppcheck is one of them. See https://invent.kde.org/sysadmin/ci-utilities/-/blob/master/config-template.yml#L21 > > Look at the results here > https://invent.kde.org/graphics/okular/-/merge_requests/544 > > Major - Either the condition 'printDialog' is redundant or there is > possible null pointer dereference: printDialog. (CWE-476) > in part/part.cpp:3341 > > Fixed: Major - Either the condition 'printDialog' is redundant or there is > possible null pointer dereference: printDialog. (CWE-476) > in part/part.cpp:3340 > > gitlab my friend, don't you think that maybe, just maybe this is the same > code and you shouldn't complain to me about it since the only change to > that file is 3000 lines away from it? > This is possibly cppcheck's fault, but yes not terribly good work there on fuzzing for line changes. > > I find it confusing, it always makes me sad lowering my productivity. > > Cheers, > Albert > > > Cheers, Ben
Re: Gitlab CI: failed unit tests vs. currently passing CI
On Sun, Jan 23, 2022 at 12:29 PM Albert Astals Cid wrote: > El diumenge, 23 de gener de 2022, a les 0:09:23 (CET), Ben Cooksley va > escriure: > > On Sun, Jan 23, 2022 at 11:29 AM Albert Astals Cid > wrote: > > > > > El dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben Cooksley va > > > escriure: > > > > EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM > Friedrich > > > > W. H. Kossebau wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > seems that Gitlab CI is currently configured to show the green > > > "Success" > > > > > checkmark for pipeline runs even if unit tests are failing. > > > > > > > > > > > > > That is correct, only compilation or other internal failures cause > the > > > > build to show a failure result. > > > > > > > > > > > > > Reasons seems to be that there Gitlab only knows Yay or Nay, > without > > > the > > > > > warning state level known from Jenkins. > > > > > > > > > > > > > Also correct. > > > > > > > > > > > > > And given that quite some projects (sadly) maintain a few long-time > > > > > failing > > > > > unit tests, having the pipeline fail on unit tests seems to have > been > > > seen > > > > > as > > > > > too aggressive > > > > > > > > > > > > Correct again. > > > > > > > > > > > > > > > > > > > > > > > This of course harms the purpose of the unit tests, when their > failures > > > > > are > > > > > only noticed weeks later, not e.g. at MR discussion time. > > > > > > > > > > > > > Gitlab does note changes in the test suite as can currently be seen > on > > > > https://invent.kde.org/frameworks/kio/-/merge_requests/708 > > > > Quoting the page: "Test summary contained 33 failed and 16 fixed > test > > > > results out of 205 total tests" > > > > > > > > It does the same thing for Code Quality - "Code quality scanning > detected > > > > 51 changes in merged results" > > > > > > Don't want to derail the confirmation, but those results are terrible, > > > they always say things changed in places not touched by the code of > the MR, > > > any idea why? > > > > > > > Unfortunately not - my only guess would be that cppcheck reports results > > slightly differently which Gitlab has issues interpreting. > > > > > > > > > > > > > > > > > > > > > > > > > Seeing how at least in KDE Frameworks first regressions sneaked in > > > without > > > > > someone noticing (nobody looks at logs when the surface shows a > green > > > > > checkmark, e.g. kcoreaddons, kwidgetsaddons, kio, purpose, krunner > on > > > > > openSUSE > > > > > and possibly more have regressed in recent weeks, see > build.kde.org) > > > this > > > > > should be something to deal with better, right? > > > > > > > > > > > > > Bhushan gave two first ideas just now on #kde-sysadmin: > > > > > > Well we can add a switch that repos can commit to saying test > > > failure is > > > > > build failure > > > > > > Another alternative is we use bot to write a comment on MR > > > > > > > > > > IMHO, to give unit tests the purpose they have, we should by > default to > > > > > let > > > > > test failures be build failures. And have projects opt out if they > > > need to > > > > > have some unit tests keep failing, instead of e.g. tagging them > with > > > > > expected > > > > > failures or handling any special environment they run into on the > CI. > > > > > > > > > > Your opinions? > > > > > > > > > > > > > The switch will need to be around the other way i'm afraid as there > are > > > > simply too many projects with broken tests right now. > > > > The best place for that switch will be in .kde-ci.yml. > > > > > > > > My only concern however would be abuse of this switch, much in the > way > > > that > > > > certain projects abuse EXCLUDE_DEPRECATED_BEFORE_AND_AT. > > > > The last thing we would want would be for people to flip this switch > and > > > > then leave their CI builds in a failing state - meaning that actual > > > > compilation failures would be missed (and then lead to CI maintenance > > > > issues) > > > > > > > > Thoughts on that? > > > > > > Test failing should mark the CI as failed, anything other than that > > > doesn't make sense. The CI did fail marking it as passed is lying to > > > ourselves. > > > > > > > We can *still* merge failed MR with failed CI, the Merge button is just > > > red, but it will work. > > > > > > > There is a big difference between "this doesn't compile" (because someone > > forgot to commit a header/etc, dependency change that isn't in place or > > because of a platform specific issue) and "some tests failed". > > What that encourages is for people to ignore the results from the CI > system > > - as they'll get used to ignoring the CI system saying something is > failing. > > I disagree, "this doesn't compile" is usually super easy to fix once > spotted, a failing test is usually more nuanced and catching it when it > starts happening is paramount. > Except when it comes to non-Linux platforms where in some cases developers are hostile to fixing issues - viewing t
Re: Gitlab CI: failed unit tests vs. currently passing CI
El diumenge, 23 de gener de 2022, a les 0:09:23 (CET), Ben Cooksley va escriure: > On Sun, Jan 23, 2022 at 11:29 AM Albert Astals Cid wrote: > > > El dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben Cooksley va > > escriure: > > > EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM Friedrich > > > W. H. Kossebau wrote: > > > > > > > Hi, > > > > > > > > > > seems that Gitlab CI is currently configured to show the green > > "Success" > > > > checkmark for pipeline runs even if unit tests are failing. > > > > > > > > > > That is correct, only compilation or other internal failures cause the > > > build to show a failure result. > > > > > > > > > > Reasons seems to be that there Gitlab only knows Yay or Nay, without > > the > > > > warning state level known from Jenkins. > > > > > > > > > > Also correct. > > > > > > > > > > And given that quite some projects (sadly) maintain a few long-time > > > > failing > > > > unit tests, having the pipeline fail on unit tests seems to have been > > seen > > > > as > > > > too aggressive > > > > > > > > > Correct again. > > > > > > > > > > > > > > > > > > This of course harms the purpose of the unit tests, when their failures > > > > are > > > > only noticed weeks later, not e.g. at MR discussion time. > > > > > > > > > > Gitlab does note changes in the test suite as can currently be seen on > > > https://invent.kde.org/frameworks/kio/-/merge_requests/708 > > > Quoting the page: "Test summary contained 33 failed and 16 fixed test > > > results out of 205 total tests" > > > > > > It does the same thing for Code Quality - "Code quality scanning detected > > > 51 changes in merged results" > > > > Don't want to derail the confirmation, but those results are terrible, > > they always say things changed in places not touched by the code of the MR, > > any idea why? > > > > Unfortunately not - my only guess would be that cppcheck reports results > slightly differently which Gitlab has issues interpreting. Can we just disable it? Look at the results here https://invent.kde.org/graphics/okular/-/merge_requests/544 Major - Either the condition 'printDialog' is redundant or there is possible null pointer dereference: printDialog. (CWE-476) in part/part.cpp:3341 Fixed: Major - Either the condition 'printDialog' is redundant or there is possible null pointer dereference: printDialog. (CWE-476) in part/part.cpp:3340 gitlab my friend, don't you think that maybe, just maybe this is the same code and you shouldn't complain to me about it since the only change to that file is 3000 lines away from it? I find it confusing, it always makes me sad lowering my productivity. Cheers, Albert
Re: Gitlab CI: failed unit tests vs. currently passing CI
El diumenge, 23 de gener de 2022, a les 0:09:23 (CET), Ben Cooksley va escriure: > On Sun, Jan 23, 2022 at 11:29 AM Albert Astals Cid wrote: > > > El dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben Cooksley va > > escriure: > > > EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM Friedrich > > > W. H. Kossebau wrote: > > > > > > > Hi, > > > > > > > > > > seems that Gitlab CI is currently configured to show the green > > "Success" > > > > checkmark for pipeline runs even if unit tests are failing. > > > > > > > > > > That is correct, only compilation or other internal failures cause the > > > build to show a failure result. > > > > > > > > > > Reasons seems to be that there Gitlab only knows Yay or Nay, without > > the > > > > warning state level known from Jenkins. > > > > > > > > > > Also correct. > > > > > > > > > > And given that quite some projects (sadly) maintain a few long-time > > > > failing > > > > unit tests, having the pipeline fail on unit tests seems to have been > > seen > > > > as > > > > too aggressive > > > > > > > > > Correct again. > > > > > > > > > > > > > > > > > > This of course harms the purpose of the unit tests, when their failures > > > > are > > > > only noticed weeks later, not e.g. at MR discussion time. > > > > > > > > > > Gitlab does note changes in the test suite as can currently be seen on > > > https://invent.kde.org/frameworks/kio/-/merge_requests/708 > > > Quoting the page: "Test summary contained 33 failed and 16 fixed test > > > results out of 205 total tests" > > > > > > It does the same thing for Code Quality - "Code quality scanning detected > > > 51 changes in merged results" > > > > Don't want to derail the confirmation, but those results are terrible, > > they always say things changed in places not touched by the code of the MR, > > any idea why? > > > > Unfortunately not - my only guess would be that cppcheck reports results > slightly differently which Gitlab has issues interpreting. > > > > > > > > > > > > > > > > > > Seeing how at least in KDE Frameworks first regressions sneaked in > > without > > > > someone noticing (nobody looks at logs when the surface shows a green > > > > checkmark, e.g. kcoreaddons, kwidgetsaddons, kio, purpose, krunner on > > > > openSUSE > > > > and possibly more have regressed in recent weeks, see build.kde.org) > > this > > > > should be something to deal with better, right? > > > > > > > > > > Bhushan gave two first ideas just now on #kde-sysadmin: > > > > > Well we can add a switch that repos can commit to saying test > > failure is > > > > build failure > > > > > Another alternative is we use bot to write a comment on MR > > > > > > > > IMHO, to give unit tests the purpose they have, we should by default to > > > > let > > > > test failures be build failures. And have projects opt out if they > > need to > > > > have some unit tests keep failing, instead of e.g. tagging them with > > > > expected > > > > failures or handling any special environment they run into on the CI. > > > > > > > > Your opinions? > > > > > > > > > > The switch will need to be around the other way i'm afraid as there are > > > simply too many projects with broken tests right now. > > > The best place for that switch will be in .kde-ci.yml. > > > > > > My only concern however would be abuse of this switch, much in the way > > that > > > certain projects abuse EXCLUDE_DEPRECATED_BEFORE_AND_AT. > > > The last thing we would want would be for people to flip this switch and > > > then leave their CI builds in a failing state - meaning that actual > > > compilation failures would be missed (and then lead to CI maintenance > > > issues) > > > > > > Thoughts on that? > > > > Test failing should mark the CI as failed, anything other than that > > doesn't make sense. The CI did fail marking it as passed is lying to > > ourselves. > > > > We can *still* merge failed MR with failed CI, the Merge button is just > > red, but it will work. > > > > There is a big difference between "this doesn't compile" (because someone > forgot to commit a header/etc, dependency change that isn't in place or > because of a platform specific issue) and "some tests failed". > What that encourages is for people to ignore the results from the CI system > - as they'll get used to ignoring the CI system saying something is failing. I disagree, "this doesn't compile" is usually super easy to fix once spotted, a failing test is usually more nuanced and catching it when it starts happening is paramount. > > While this is not such a big deal for Linux, it is a massive deal for the > smaller platforms that far less people run. > > Saying you can merge when the CI says it is failing is setting ourselves up > for failure. I don't see how hiding (because of the super low visibility they have) that the tests are failing, or even worse have started failing (status quo and as far as I understand your suggestion) is any better. Cheers, Albert > > > > Maybe
Re: Gitlab CI: failed unit tests vs. currently passing CI
On Sun, Jan 23, 2022 at 11:29 AM Albert Astals Cid wrote: > El dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben Cooksley va > escriure: > > EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM Friedrich > > W. H. Kossebau wrote: > > > > > Hi, > > > > > > > seems that Gitlab CI is currently configured to show the green > "Success" > > > checkmark for pipeline runs even if unit tests are failing. > > > > > > > That is correct, only compilation or other internal failures cause the > > build to show a failure result. > > > > > > > Reasons seems to be that there Gitlab only knows Yay or Nay, without > the > > > warning state level known from Jenkins. > > > > > > > Also correct. > > > > > > > And given that quite some projects (sadly) maintain a few long-time > > > failing > > > unit tests, having the pipeline fail on unit tests seems to have been > seen > > > as > > > too aggressive > > > > > > Correct again. > > > > > > > > > > > > > This of course harms the purpose of the unit tests, when their failures > > > are > > > only noticed weeks later, not e.g. at MR discussion time. > > > > > > > Gitlab does note changes in the test suite as can currently be seen on > > https://invent.kde.org/frameworks/kio/-/merge_requests/708 > > Quoting the page: "Test summary contained 33 failed and 16 fixed test > > results out of 205 total tests" > > > > It does the same thing for Code Quality - "Code quality scanning detected > > 51 changes in merged results" > > Don't want to derail the confirmation, but those results are terrible, > they always say things changed in places not touched by the code of the MR, > any idea why? > Unfortunately not - my only guess would be that cppcheck reports results slightly differently which Gitlab has issues interpreting. > > > > > > > > > > > Seeing how at least in KDE Frameworks first regressions sneaked in > without > > > someone noticing (nobody looks at logs when the surface shows a green > > > checkmark, e.g. kcoreaddons, kwidgetsaddons, kio, purpose, krunner on > > > openSUSE > > > and possibly more have regressed in recent weeks, see build.kde.org) > this > > > should be something to deal with better, right? > > > > > > > Bhushan gave two first ideas just now on #kde-sysadmin: > > > > Well we can add a switch that repos can commit to saying test > failure is > > > build failure > > > > Another alternative is we use bot to write a comment on MR > > > > > > IMHO, to give unit tests the purpose they have, we should by default to > > > let > > > test failures be build failures. And have projects opt out if they > need to > > > have some unit tests keep failing, instead of e.g. tagging them with > > > expected > > > failures or handling any special environment they run into on the CI. > > > > > > Your opinions? > > > > > > > The switch will need to be around the other way i'm afraid as there are > > simply too many projects with broken tests right now. > > The best place for that switch will be in .kde-ci.yml. > > > > My only concern however would be abuse of this switch, much in the way > that > > certain projects abuse EXCLUDE_DEPRECATED_BEFORE_AND_AT. > > The last thing we would want would be for people to flip this switch and > > then leave their CI builds in a failing state - meaning that actual > > compilation failures would be missed (and then lead to CI maintenance > > issues) > > > > Thoughts on that? > > Test failing should mark the CI as failed, anything other than that > doesn't make sense. The CI did fail marking it as passed is lying to > ourselves. > We can *still* merge failed MR with failed CI, the Merge button is just > red, but it will work. > There is a big difference between "this doesn't compile" (because someone forgot to commit a header/etc, dependency change that isn't in place or because of a platform specific issue) and "some tests failed". What that encourages is for people to ignore the results from the CI system - as they'll get used to ignoring the CI system saying something is failing. While this is not such a big deal for Linux, it is a massive deal for the smaller platforms that far less people run. Saying you can merge when the CI says it is failing is setting ourselves up for failure. > Maybe this red button will convince people to fix their tests. (one can > hope, right?) > > Of course if we do this change it should happen after we've done that > change that fixes the test failing because of however gitlab CI is set-up > (you mentioned we had to wait for Jenkins to be disabled for that) > > Cheers, > Albert > Regards, Ben > > > > > > > > > > > Cheers > > > Friedrich > > > > > > > Cheers, > > Ben > > > > > > >
Re: Gitlab CI: failed unit tests vs. currently passing CI
El dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben Cooksley va escriure: > EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM Friedrich > W. H. Kossebau wrote: > > > Hi, > > > > seems that Gitlab CI is currently configured to show the green "Success" > > checkmark for pipeline runs even if unit tests are failing. > > > > That is correct, only compilation or other internal failures cause the > build to show a failure result. > > > > Reasons seems to be that there Gitlab only knows Yay or Nay, without the > > warning state level known from Jenkins. > > > > Also correct. > > > > And given that quite some projects (sadly) maintain a few long-time > > failing > > unit tests, having the pipeline fail on unit tests seems to have been seen > > as > > too aggressive > > > Correct again. > > > > > > > > This of course harms the purpose of the unit tests, when their failures > > are > > only noticed weeks later, not e.g. at MR discussion time. > > > > Gitlab does note changes in the test suite as can currently be seen on > https://invent.kde.org/frameworks/kio/-/merge_requests/708 > Quoting the page: "Test summary contained 33 failed and 16 fixed test > results out of 205 total tests" > > It does the same thing for Code Quality - "Code quality scanning detected > 51 changes in merged results" Don't want to derail the confirmation, but those results are terrible, they always say things changed in places not touched by the code of the MR, any idea why? > > > > > > Seeing how at least in KDE Frameworks first regressions sneaked in without > > someone noticing (nobody looks at logs when the surface shows a green > > checkmark, e.g. kcoreaddons, kwidgetsaddons, kio, purpose, krunner on > > openSUSE > > and possibly more have regressed in recent weeks, see build.kde.org) this > > should be something to deal with better, right? > > > > Bhushan gave two first ideas just now on #kde-sysadmin: > > > Well we can add a switch that repos can commit to saying test failure is > > build failure > > > Another alternative is we use bot to write a comment on MR > > > > IMHO, to give unit tests the purpose they have, we should by default to > > let > > test failures be build failures. And have projects opt out if they need to > > have some unit tests keep failing, instead of e.g. tagging them with > > expected > > failures or handling any special environment they run into on the CI. > > > > Your opinions? > > > > The switch will need to be around the other way i'm afraid as there are > simply too many projects with broken tests right now. > The best place for that switch will be in .kde-ci.yml. > > My only concern however would be abuse of this switch, much in the way that > certain projects abuse EXCLUDE_DEPRECATED_BEFORE_AND_AT. > The last thing we would want would be for people to flip this switch and > then leave their CI builds in a failing state - meaning that actual > compilation failures would be missed (and then lead to CI maintenance > issues) > > Thoughts on that? Test failing should mark the CI as failed, anything other than that doesn't make sense. The CI did fail marking it as passed is lying to ourselves. We can *still* merge failed MR with failed CI, the Merge button is just red, but it will work. Maybe this red button will convince people to fix their tests. (one can hope, right?) Of course if we do this change it should happen after we've done that change that fixes the test failing because of however gitlab CI is set-up (you mentioned we had to wait for Jenkins to be disabled for that) Cheers, Albert > > > > > > Cheers > > Friedrich > > > > Cheers, > Ben >
Re: Gitlab CI: failed unit tests vs. currently passing CI
Important topic, thanks for bringing this up! On Samstag, 22. Januar 2022 01:31:13 CET Friedrich W. H. Kossebau wrote: > Bhushan gave two first ideas just now on #kde-sysadmin: > > Well we can add a switch that repos can commit to saying test failure is > > build failure > > > Another alternative is we use bot to write a comment on MR > > IMHO, to give unit tests the purpose they have, we should by default to let > test failures be build failures. And have projects opt out if they need to > have some unit tests keep failing, instead of e.g. tagging them with > expected failures or handling any special environment they run into on the > CI. I'd also appreciate the option to make test failures build failures. I do agree with Ben that at this point it's probably not practical yet to make that opt-out. Even more so when that switch is global rather than per build type, thinking about the harder to support platforms like Windows and (at this point) Linux/Qt6 for example. Of course I agree that the eventual goal should be that this is on for all modules on all platforms, but we wont get there over night. Regards, Volker signature.asc Description: This is a digitally signed message part.
Re: Gitlab CI: failed unit tests vs. currently passing CI
EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM Friedrich W. H. Kossebau wrote: > Hi, > seems that Gitlab CI is currently configured to show the green "Success" > checkmark for pipeline runs even if unit tests are failing. > That is correct, only compilation or other internal failures cause the build to show a failure result. > Reasons seems to be that there Gitlab only knows Yay or Nay, without the > warning state level known from Jenkins. > Also correct. > And given that quite some projects (sadly) maintain a few long-time > failing > unit tests, having the pipeline fail on unit tests seems to have been seen > as > too aggressive Correct again. > > > This of course harms the purpose of the unit tests, when their failures > are > only noticed weeks later, not e.g. at MR discussion time. > Gitlab does note changes in the test suite as can currently be seen on https://invent.kde.org/frameworks/kio/-/merge_requests/708 Quoting the page: "Test summary contained 33 failed and 16 fixed test results out of 205 total tests" It does the same thing for Code Quality - "Code quality scanning detected 51 changes in merged results" > > Seeing how at least in KDE Frameworks first regressions sneaked in without > someone noticing (nobody looks at logs when the surface shows a green > checkmark, e.g. kcoreaddons, kwidgetsaddons, kio, purpose, krunner on > openSUSE > and possibly more have regressed in recent weeks, see build.kde.org) this > should be something to deal with better, right? > Bhushan gave two first ideas just now on #kde-sysadmin: > > Well we can add a switch that repos can commit to saying test failure is > build failure > > Another alternative is we use bot to write a comment on MR > > IMHO, to give unit tests the purpose they have, we should by default to > let > test failures be build failures. And have projects opt out if they need to > have some unit tests keep failing, instead of e.g. tagging them with > expected > failures or handling any special environment they run into on the CI. > > Your opinions? > The switch will need to be around the other way i'm afraid as there are simply too many projects with broken tests right now. The best place for that switch will be in .kde-ci.yml. My only concern however would be abuse of this switch, much in the way that certain projects abuse EXCLUDE_DEPRECATED_BEFORE_AND_AT. The last thing we would want would be for people to flip this switch and then leave their CI builds in a failing state - meaning that actual compilation failures would be missed (and then lead to CI maintenance issues) Thoughts on that? > > Cheers > Friedrich > Cheers, Ben
