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 <aa...@kde.org> 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 <kosse...@kde.org> 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 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 > > > > > > > > > > > > > >