refactor!: Propagate free-to-bound conversion error #739
Conversation
Previously, if a free parameter set was given that was not on-surface, a FATAL error message was given, but the parameters were still returned. Client code is then unable to tell something went wrong. This changes the error to propagate, so it can hopefully be caught by the top caller.
Core/include/Acts/EventData/detail/TransformationFreeToBound.hpp
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #739 +/- ##
==========================================
- Coverage 48.96% 48.87% -0.10%
==========================================
Files 325 325
Lines 16639 16670 +31
Branches 7762 7783 +21
==========================================
Hits 8147 8147
- Misses 3042 3058 +16
- Partials 5450 5465 +15
Continue to review full report at Codecov.
|
|
Ok, there we go. The unit test that I now added fails because of the FATAL that is logged. @HadrienG2 what would be your preferred solution here? Remove the FATAL, demote it to ERROR? |
|
Hmmm, I didn't expect that indeed. Demoting to ERROR won't work, because with current CI settings fail-on-error will trigger all the way down to WARNING, so you need to use INFO level or less. Which seems inappropriate. Try-catching the fail-on-error exception could work, but will not allow you to check that a correct Result is returned when fail-on-error is disabled. So what you probably need here is a mechanism to locally disable fail-on-error for the duration of the test... which means making the fail-on-error level dynamic (yay global variable). Or you can just remove the offending log or demote it to INFO, if it doesn't bring that much to have it FATAL. |
|
Basically, the reason why this is tricky is that we have an operation that is an error from the point of view of the parameter conversion code, but not an error from the point of view of the whole program (unit test), whereas the current fail-on-error logic considers log severity to be a program-wide property. I have no idea how to pass down the information that some local errors are not program-wide errors without indefinitely delaying logs and making every Acts component that calls code that emits logs care about fail-on-error (which would certainly be excessive), so since unit tests that exercise failures are rare, I guess the least bad option would be to instead have a way for them to say "errors are expected here, and that's fine by me, I'll handle them". |
|
Honestly, I'm considering to just remove the fatal completely. Now that the error bubbles up, it's not critical to to have this anymore I suppose, and it uses a hack to get hold of a logger anyway so I'd be happy to remove that. Does that seem reasonable? |
|
From the user side, I agree with Paul. The logging doesn't provide anything anymore if the exception can be handled by the user, so it doesn't serve a purpose if the result can be caught. |
Previously, if a free parameter set was given that was not on-surface, a FATAL error message was given, but the parameters were still returned. Client code is then unable to tell something went wrong. This changes the error to propagate, so it can hopefully be caught by the top caller.
Addresses #579, but I'm not sure it fixes it, really.
/cc @osbornjd