Fixes ImageMetricsTestCase

[gastaldi]

This fixes the annoying test failure with org.opentest4j.AssertionFailedError: Expected analysis_results.fields.reflection to be within range [163 +- 3%] but was 168 ==> expected: <true> but was: <false>

[quarkus-bot]

/cc @gsmet (hibernate-orm), @yrodiere (hibernate-orm)

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit eb3841d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[geoand]

cc @zakkak

[yrodiere]

Works for me.

Also, I wonder if these tests even make sense in our current situation... They would make sense if we were able to react promptly to a failure to analyze and propose a fix if necessary, but AFAICS everybody involved (me included) has higher priorities, so... Looks like a losing battle.

[zakkak]

This fixes the annoying test failure with org.opentest4j.AssertionFailedError: Expected analysis_results.fields.reflection to be within range [163 +- 3%] but was 168 ==> expected: but was:

@gastaldi where did this show up? Is it in Quarkus' CI?

If yes what triggered it?

If no you have the option to (and should) disable these tests by setting the QUARKUS_NATIVE_IT_SKIP_VERIFY_IMAGE_METRICS environment variable.

Works for me.

Also, I wonder if these tests even make sense in our current situation...

@yrodiere can you clarify what is the current situation? Do you mean that such issues keep popping up or something else?

They would make sense if we were able to react promptly to a failure to analyze and propose a fix if necessary, but AFAICS everybody involved (me included) has higher priorities, so... Looks like a losing battle.

I can see how this might be frustrating, but it's necessary to avoid ending up with issues like #38683 (which would probably have been caught much earlier if the ImageMetricsITCase was enabled for the kubernetes-client integration tests as well.) and prevent the native executables becoming bigger and bigger as shown in #38600 (comment)

Some ideas to improve the current situation (as I understand it):

1. We could relax the thresholds to make the tests more forgiving. This will result in less failures but it will also make it harder to identify what caused the increase since the failure might be the result of multiple changes (given the larger threshold).

2. We could move these tests in a separate job and allow them to fail without making the CI red, but a warning should still be printed (or a GH issue get updated) to make sure we don't miss the failures. What I fear is that if we follow an approach like this people will just ignore these failures even when they should not.

[zakkak]

@gastaldi where did this show up? Is it in Quarkus' CI?

If this was for #38919 then that has already been addressed with #39026, thus this PR should be reverted IMO.

[yrodiere]

@yrodiere can you clarify what is the current situation? Do you mean that such issues keep popping up or something else?

I was mostly talking about my own situation TBH: I have too many things on my plate already, so can't really spare cycles on investigating these failures (even though technically I'm "responsible" for these extensions). If you can affort to prioritize this yourself though, I can definitely ping you.

Some ideas to improve the current situation (as I understand it):

I think you're spot on on the downsides and they probably would make the situation worse, unfortunately...

The only zero-downside improvement I can see would be to find a way to notify "code owners" when tests of their "owned" extension starts failing on non-PR builds. That's probably something the bot could handle, based on the already-configured automatic issue labeling, right @gsmet?

With something like that I would at least have known of the failure earlier and could have pinged you. Though if we're being honest I failed to ping you even after I noticed the failure, so that would still require more rigor from me.

[gastaldi]

@gastaldi where did this show up? Is it in Quarkus' CI?

A lot of PRs were reporting it, like #39009 (comment) and #39015 (comment), hence why I created it.

If this was for #38919 then that has already been addressed with #39026, thus this PR should be reverted IMO.

Great, I'll revert it if it's already fixed then

[zakkak]

I was mostly talking about my own situation TBH: I have too many things on my plate already, so can't really spare cycles on investigating these failures (even though technically I'm "responsible" for these extensions). If you can affort to prioritize this yourself though, I can definitely ping you.

Thanks for the clarification. Unfortunately that's not going to work either. The aim of the tests is to let you (the Quarkus developers) know that your code changes are affecting the native image binaries. Even if I notice the change, in most cases I can't really tell if that's OK or not, e.g. you might introduce some new functionality which totally justifies the increase of the binary size. On the other hand you might just use some new API like in #38820 that will fetch more things. In that case I understand that it's not always trivial (for both of us) to judge whether the increase is welcome or not.

The only zero-downside improvement I can see would be to find a way to notify "code owners" when tests of their "owned" extension starts failing on non-PR builds.

That shouldn't happen though, right? If the tests start failing without first failing in a PR then the change that triggered the failures is external to the Quarkus repository (it's most likely a change in the builder image, so it's an issue for us, the mandrel team). Do you have any examples in mind?

That's probably something the bot could handle, based on the already-configured automatic issue labeling, right @gsmet?

+1 it wouldn't hurt if the bot could ping me on such cases, as long as that doesn't mean I am always "assigned" resolving the issue.

With something like that I would at least have known of the failure earlier and could have pinged you. Though if we're being honest I failed to ping you even after I noticed the failure, so that would still require more rigor from me.

If we are still talking about the specific failures (and not some earlier issue) even though they got ignored in the PR that introduced them, they quickly came to our attention (breaking PR merged on the 20th, issue tracking the issue created on the 21st thanks to @gsmet's ping) and eventually got fixed after a few days (on the 27th).

IMO if we/you:

1. avoid merging PRs that break these tests without either addressing the issue or adjusting the thresholds,
2. ping me or galderz (if I don't seem to be available) if you can't understand why your code change has an impact on the binary size.
3. ping the mandrel team (not just me as I am a single point of failure :) ), i.e. zakkak, galderz, jerboaa, or karm as soon as a failure not linked to a Quarkus PR pops up.

Things shouldn't break or stay broken for long periods of time.

[yrodiere]

That shouldn't happen though, right? If the tests start failing without first failing in a PR then the change that triggered the failures is external to the Quarkus repository (it's most likely a change in the builder image, so it's an issue for us, the mandrel team). Do you have any examples in mind?

Failures external to the Quarkus repository are the only one I'm concerned about TBH. If I break stuff in my PR, that PR won't get merged until I find the time to debug that :)

If we are still talking about the specific failures (and not some earlier issue) even though they got ignored in the PR that introduced them, they quickly came to our attention (breaking PR merged on the 20th, issue tracking the issue created on the 21st thanks to @gsmet's ping) and eventually got fixed after a few days (on the 27th).

I thought this went under your radar since @gastaldi submitted the fix, but yeah it seems the fix you submitted in parallel addressed the same issue.

Things shouldn't break or stay broken for long periods of time.

Alright, well let's try that :)

(not just me as I am a single point of failure :) ), i.e. zakkak, galderz, jerboaa, or karm

I wish we had GitHub teams for that... and not just for the Mandrel team, as it's definitely a problem on other extensions.