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 <aa...@kde.org> 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 <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. > > > > Except when it comes to non-Linux platforms where in some cases developers > are hostile to fixing issues - viewing those platforms as someone else's > problem. > One of the benefits we were expecting to get out of Gitlab CI was that > build failures on platforms like Windows wouldn't end up being merged and > would be fixed before it lands. > > You are now wanting to remove that.
I can see where you're coming from, but just let it be clear that this is *not* what I want, what I want is more strictness "green means actually working (i.e. test pass), not just compiling" not less. Cheers, Albert > > In that case I guess we should remove everything but Linux CI - because the > cost of maintaining it will be too high. > > > > > > > > > > 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. > > > > If a project has all passing tests and has a commitment to ensure they > remain passing, then I have no problem in principle with that project being > allowed to make failing tests mean a failing CI run. > Code should never be merged when the CI run fails though. > > We should never normalise merging when the CI reports a failure - it makes > pre-merge CI pointless. > > > > > > Cheers, > > Albert > > > > Thanks, > Ben > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
