[SPARK-54325][TESTS] Use custom matcher in ExecutorSuite testThrowable #53022
Conversation
| isFatal: Boolean): Unit = { | ||
| import Executor.isFatalError | ||
|
|
||
| class BeEqualToIsFatal(isFatal: Boolean) extends Matcher[Throwable] { |
There was a problem hiding this comment.
Can we give it a more descriptive name?
There was a problem hiding this comment.
@holdenk do you have a suggestion on how to name it? Note the usage, for example:
errorInThreadPool(errorInGuavaCache(e)) should beEqualToIsFatal(depthToCheck >= 3 && isFatal)
There was a problem hiding this comment.
So the usage of this is to see if an error is a fatal error or not (depending on the boolean flag passed in) right? shouldBeFatalError(true or false) makes a bit more sense to be me from a naming point of view, WDYT?
There was a problem hiding this comment.
So the usage of this is to see if an error is a fatal error or not (depending on the boolean flag passed in) right?
Almost right, the usage is to check if the unwrapped error is a fatal error or not, depending on the depth and the boolean flag.
shouldBeFatalError(true or false)
- "should" must not be part of the class name. It is already part of the matcher check
- I am OK with "beFatalError", though "e should beFatalError(false)" means "e should not be fatal error".
The difference between 2 is
- The initial naming corresponds to "is equal to
isFatalflag" - The new naming corresponds to "is fatal error (true or false)".
IMO, the first option is preferable, though I am OK with both options.
@holdenk WDYT?
|
@holdenk Please take a look |
2 participants
What changes were proposed in this pull request?
Use custom
BeEqualToIsFatalmatcher instead ofassertinExecutorSuite.testThrowableWhy are the changes needed?
assertexpression and make check more readableDoes this PR introduce any user-facing change?
No
How was this patch tested?
execute affected test
Was this patch authored or co-authored using generative AI tooling?
No