<div dir="ltr"><div dir="ltr">On Sun, Jan 23, 2022 at 11:29 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 dissabte, 22 de gener de 2022, a les 6:11:29 (CET), Ben Cooksley va escriure:<br>
> EXCLUDE_DEPRECATED_BEFORE_AND_ATOn Sat, Jan 22, 2022 at 1:31 PM 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 "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 the<br>
> build to show a failure result.<br>
> <br>
> <br>
> > Reasons seems to be that there Gitlab only knows Yay or Nay, without 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 been 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 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 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 test<br>
> results out of 205 total tests"<br>
> <br>
> It does the same thing for Code Quality - "Code quality scanning detected<br>
> 51 changes in merged results"<br>
<br>
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?<br></blockquote><div><br></div><div>Unfortunately not - my only guess would be that cppcheck reports results slightly differently which Gitlab has issues interpreting.</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>
> <br>
> ><br>
> > Seeing how at least in KDE Frameworks first regressions sneaked in without<br>
> > someone noticing (nobody looks at logs when the surface shows a green<br>
> > checkmark, e.g. kcoreaddons, kwidgetsaddons, kio, purpose, krunner on<br>
> > openSUSE<br>
> > and possibly more have regressed in recent weeks, see <a href="http://build.kde.org" rel="noreferrer" target="_blank">build.kde.org</a>) 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 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 default to<br>
> > let<br>
> > test failures be build failures. And have projects opt out if they need to<br>
> > have some unit tests keep failing, instead of e.g. tagging them with<br>
> > expected<br>
> > failures or handling any special environment they run into on the CI.<br>
> ><br>
> > Your opinions?<br>
> ><br>
> <br>
> The switch will need to be around the other way i'm afraid as there 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 way 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 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 doesn't make sense. The CI did fail marking it as passed is lying to ourselves. </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
We can *still* merge failed MR with failed CI, the Merge button is just red, but it will work.<br></blockquote><div><br></div><div>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".</div><div>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.</div><div><br></div><div>While this is not such a big deal for Linux, it is a massive deal for the smaller platforms that far less people run.</div><div><br></div><div>Saying you can merge when the CI says it is failing is setting ourselves up for failure.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Maybe this red button will convince people to fix their tests. (one can hope, right?)<br>
<br>
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)<br>
<br>
Cheers,<br>
  Albert<br></blockquote><div><br></div><div>Regards,</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>
> <br>
> ><br>
> > Cheers<br>
> > Friedrich<br>
> ><br>
> <br>
> Cheers,<br>
> Ben<br>
> <br>
<br>
<br>
<br>
<br>
</blockquote></div></div>