<div dir="ltr"><div dir="ltr">On Mon, Jan 24, 2022 at 12:56 AM Albert Astals Cid <<a href="mailto:aacid@kde.org">aacid@kde.org</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">El diumenge, 23 de gener de 2022, a les 1:59:01 (CET), Ben Cooksley va escriure:<br>
> On Sun, Jan 23, 2022 at 12:29 PM Albert Astals Cid <<a href="mailto:aacid@kde.org" target="_blank">aacid@kde.org</a>> wrote:<br>
> <br>
> > El diumenge, 23 de gener de 2022, a les 0:09:23 (CET), Ben Cooksley va<br>
> > escriure:<br>
> > > On Sun, Jan 23, 2022 at 11:29 AM Albert Astals Cid <<a href="mailto:aacid@kde.org" target="_blank">aacid@kde.org</a>><br>
> > wrote:<br>
> > ><br>
> > > > El dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben Cooksley va<br>
> > > > escriure:<br>
> > > > > EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM<br>
> > Friedrich<br>
> > > > > W. H. Kossebau <<a href="mailto:kossebau@kde.org" target="_blank">kossebau@kde.org</a>> wrote:<br>
> > > > ><br>
> > > > > > Hi,<br>
> > > > ><br>
> > > > ><br>
> > > > > > seems that Gitlab CI is currently configured to show the green<br>
> > > > "Success"<br>
> > > > > > checkmark for pipeline runs even if unit tests are failing.<br>
> > > > > ><br>
> > > > ><br>
> > > > > That is correct, only compilation or other internal failures cause<br>
> > the<br>
> > > > > build to show a failure result.<br>
> > > > ><br>
> > > > ><br>
> > > > > > Reasons seems to be that there Gitlab only knows Yay or Nay,<br>
> > without<br>
> > > > the<br>
> > > > > > warning state level known from Jenkins.<br>
> > > > > ><br>
> > > > ><br>
> > > > > Also correct.<br>
> > > > ><br>
> > > > ><br>
> > > > > > And given that quite some projects (sadly) maintain a few long-time<br>
> > > > > > failing<br>
> > > > > > unit tests, having the pipeline fail on unit tests seems to have<br>
> > been<br>
> > > > seen<br>
> > > > > > as<br>
> > > > > > too aggressive<br>
> > > > ><br>
> > > > ><br>
> > > > > Correct again.<br>
> > > > ><br>
> > > > ><br>
> > > > > ><br>
> > > > > ><br>
> > > > > > This of course harms the purpose of the unit tests, when their<br>
> > failures<br>
> > > > > > are<br>
> > > > > > only noticed weeks later, not e.g. at MR discussion time.<br>
> > > > > ><br>
> > > > ><br>
> > > > > Gitlab does note changes in the test suite as can currently be seen<br>
> > on<br>
> > > > > <a href="https://invent.kde.org/frameworks/kio/-/merge_requests/708" rel="noreferrer" target="_blank">https://invent.kde.org/frameworks/kio/-/merge_requests/708</a><br>
> > > > > Quoting the page:  "Test summary contained 33 failed and 16 fixed<br>
> > test<br>
> > > > > results out of 205 total tests"<br>
> > > > ><br>
> > > > > It does the same thing for Code Quality - "Code quality scanning<br>
> > detected<br>
> > > > > 51 changes in merged results"<br>
> > > ><br>
> > > > Don't want to derail the confirmation, but those results are terrible,<br>
> > > > they always say things changed in places not touched by the code of<br>
> > the MR,<br>
> > > > any idea why?<br>
> > > ><br>
> > ><br>
> > > Unfortunately not - my only guess would be that cppcheck reports results<br>
> > > slightly differently which Gitlab has issues interpreting.<br>
> > ><br>
> > ><br>
> > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > ><br>
> > > > > > Seeing how at least in KDE Frameworks first regressions sneaked in<br>
> > > > without<br>
> > > > > > someone noticing (nobody looks at logs when the surface shows a<br>
> > green<br>
> > > > > > checkmark, e.g. kcoreaddons, kwidgetsaddons, kio, purpose, krunner<br>
> > on<br>
> > > > > > openSUSE<br>
> > > > > > and possibly more have regressed in recent weeks, see<br>
> > <a href="http://build.kde.org" rel="noreferrer" target="_blank">build.kde.org</a>)<br>
> > > > this<br>
> > > > > > should be something to deal with better, right?<br>
> > > > ><br>
> > > > ><br>
> > > > > > Bhushan gave two first ideas just now on #kde-sysadmin:<br>
> > > > > > > Well we can add a switch that repos can commit to saying test<br>
> > > > failure is<br>
> > > > > > build failure<br>
> > > > > > > Another alternative is we use bot to write a comment on MR<br>
> > > > > ><br>
> > > > > > IMHO, to give unit tests the purpose they have, we should by<br>
> > default to<br>
> > > > > > let<br>
> > > > > > test failures be build failures. And have projects opt out if they<br>
> > > > need to<br>
> > > > > > have some unit tests keep failing, instead of e.g. tagging them<br>
> > with<br>
> > > > > > expected<br>
> > > > > > failures or handling any special environment they run into on the<br>
> > CI.<br>
> > > > > ><br>
> > > > > > Your opinions?<br>
> > > > > ><br>
> > > > ><br>
> > > > > The switch will need to be around the other way i'm afraid as there<br>
> > are<br>
> > > > > simply too many projects with broken tests right now.<br>
> > > > > The best place for that switch will be in .kde-ci.yml.<br>
> > > > ><br>
> > > > > My only concern however would be abuse of this switch, much in the<br>
> > way<br>
> > > > that<br>
> > > > > certain projects abuse EXCLUDE_DEPRECATED_BEFORE_AND_AT.<br>
> > > > > The last thing we would want would be for people to flip this switch<br>
> > and<br>
> > > > > then leave their CI builds in a failing state - meaning that actual<br>
> > > > > compilation failures would be missed (and then lead to CI maintenance<br>
> > > > > issues)<br>
> > > > ><br>
> > > > > Thoughts on that?<br>
> > > ><br>
> > > > Test failing should mark the CI as failed, anything other than that<br>
> > > > doesn't make sense. The CI did fail marking it as passed is lying to<br>
> > > > ourselves.<br>
> > ><br>
> > ><br>
> > > > We can *still* merge failed MR with failed CI, the Merge button is just<br>
> > > > red, but it will work.<br>
> > > ><br>
> > ><br>
> > > There is a big difference between "this doesn't compile" (because someone<br>
> > > forgot to commit a header/etc, dependency change that isn't in place or<br>
> > > because of a platform specific issue) and "some tests failed".<br>
> > > What that encourages is for people to ignore the results from the CI<br>
> > system<br>
> > > - as they'll get used to ignoring the CI system saying something is<br>
> > failing.<br>
> ><br>
> > I disagree, "this doesn't compile" is usually super easy to fix once<br>
> > spotted, a failing test is usually more nuanced and catching it when it<br>
> > starts happening is paramount.<br>
> ><br>
> <br>
> Except when it comes to non-Linux platforms where in some cases developers<br>
> are hostile to fixing issues - viewing those platforms as someone else's<br>
> problem.<br>
> One of the benefits we were expecting to get out of Gitlab CI was that<br>
> build failures on platforms like Windows wouldn't end up being merged and<br>
> would be fixed before it lands.<br>
> <br>
> You are now wanting to remove that.<br>
<br>
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.<br></blockquote><div><br></div><div>I'm happy to provide that strictness - only on the basis that the project is ready for that level of strictness.</div><div>Otherwise developers will just ignore failing CI because they'll think it is just the tests failing and won't look more closely.</div><div><br></div><div>If a project has broken tests on any platform then it is not ready for more strict CI and should continue to only have compilation taken into account as to whether a build fails or not.</div><div>The information on failing/passing tests is still presented within the Gitlab UI.</div><div><br></div><div>A number of our projects are not ready for more strict CI - with Windows being a key stumbling block for a good proportion of Frameworks.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Cheers,<br>
  Albert<br></blockquote><div><br></div><div>Cheers,</div><div>Ben</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
> In that case I guess we should remove everything but Linux CI - because the<br>
> cost of maintaining it will be too high.<br>
> <br>
> <br>
> ><br>
> > ><br>
> > > While this is not such a big deal for Linux, it is a massive deal for the<br>
> > > smaller platforms that far less people run.<br>
> > ><br>
> > > Saying you can merge when the CI says it is failing is setting ourselves<br>
> > up<br>
> > > for failure.<br>
> ><br>
> > I don't see how hiding (because of the super low visibility they have)<br>
> > that the tests are failing, or even worse have started failing (status quo<br>
> > and as far as I understand your suggestion) is any better.<br>
> ><br>
> <br>
> If a project has all passing tests and has a commitment to ensure they<br>
> remain passing, then I have no problem in principle with that project being<br>
> allowed to make failing tests mean a failing CI run.<br>
> Code should never be merged when the CI run fails though.<br>
> <br>
> We should never normalise merging when the CI reports a failure - it makes<br>
> pre-merge CI pointless.<br>
> <br>
> <br>
> ><br>
> > Cheers,<br>
> >   Albert<br>
> ><br>
> <br>
> Thanks,<br>
> Ben<br>
> <br>
> <br>
> ><br>
> > ><br>
> > ><br>
> > > > Maybe this red button will convince people to fix their tests. (one can<br>
> > > > hope, right?)<br>
> > > ><br>
> > > > Of course if we do this change it should happen after we've done that<br>
> > > > change that fixes the test failing because of however gitlab CI is<br>
> > set-up<br>
> > > > (you mentioned we had to wait for Jenkins to be disabled for that)<br>
> > > ><br>
> > > > Cheers,<br>
> > > >   Albert<br>
> > > ><br>
> > ><br>
> > > Regards,<br>
> > > Ben<br>
> > ><br>
> > ><br>
> > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > ><br>
> > > > > > Cheers<br>
> > > > > > Friedrich<br>
> > > > > ><br>
> > > > ><br>
> > > > > Cheers,<br>
> > > > > Ben<br>
> > > > ><br>
> > > ><br>
> > > ><br>
> > > ><br>
> > > ><br>
> > > ><br>
> > ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> <br>
<br>
<br>
<br>
<br>
</blockquote></div></div>