[RFC] Extract failed test debugger code into its own utiltiy class #421
Conversation
| @@ -38,6 +35,11 @@ | |||
| import org.testfx.service.finder.NodeFinder; | |||
| import org.testfx.service.query.NodeQuery; | |||
|
|
|||
| /** | |||
| * All TestFX tests should use {@link #verifyThat(Node, Matcher, Function)} when writing tests, | |||
There was a problem hiding this comment.
Maybe we should be explicit and say "All internal TestFX tests"
There was a problem hiding this comment.
Why? I really meant all tests should use the verifyThat(Node, Matcher)-related assertions because if something goes wrong, one can quickly add an error handler (type now becomes verifyThat(Node, Matcher, Function)) that provides additional info to help one debug it. This applies to both TestFX contributors and users of this test library
There was a problem hiding this comment.
Perhaps I should reword it to explain that better?
JordanMartinez
changed the title
|
Before merging, this PR should include tests that prove the code works.... |
JordanMartinez
changed the title
| private static final String TAB = " "; | ||
|
|
||
| private static final MouseButton MOUSE_BUTTON = MouseButton.PRIMARY; | ||
| private static final KeyCode KEY = KeyCode.A; |
There was a problem hiding this comment.
I don't think that these fields add anything useful. I understand if the intention is to make it clear that it can be any mouse button or key, but I think it complicates reading the test.
There was a problem hiding this comment.
If the test's assertion should fail, I need to release the mouse button or key that was pressed. Rather than defining that in two separate places that can change, I define it once here and refer to it twice (1x in test, 1x in cleanup).
| }; | ||
| } | ||
|
|
||
| private abstract class RemoveBlankMessageMatcher extends BaseMatcher<String> { |
There was a problem hiding this comment.
What exactly does this class do? Having a hard time understanding it.
There was a problem hiding this comment.
It removes the blank part (i.e. the the quotes part in `"Expected: \n\tbut: "") from the returned error's message since I can't get rid of that myself but still need a better way to determine whether a test worked or not.
I don't think it's the best designed class because I initially made it do something else before reworking it a bit. The ">>>message<<<" was necessary for debugging my tests until I replaced newline characters with the "\n" string. I'm not sure if it's needed anymore.
There was a problem hiding this comment.
I just realized that this isn't actually needed. Why? Because there's a much easier way to remove the blank part by taking the first function f and getting rid of the error message via f.compose(sb -> new StringBuilder()).
| } | ||
|
|
||
| private Matcher<Object> alwaysFail() { | ||
| return new AlwaysFailsMatcher(); |
There was a problem hiding this comment.
Could this be replaced by returning an anonymous inner class that extends Hamcrest's IsNot matcher with not() overriden to always return true? Would that make things any clearer? Not sure.
There was a problem hiding this comment.
I'm not familiar with Hamcrest and their matchers, so I ran my own after looking at the ones created in TestFX itself.
It probably would make things clearer if a developer reading through these test knows Hamcrest well.
There was a problem hiding this comment.
I looked into it. IsNot requires another Matcher to be passed into it. So, I think it's just easier to leave it as is. Besides, it also overrides the describeMismatch() to remove the "was" that gets appended to the description.
|
|
||
| private AssertionError getThrownErrorPostMapper(Function<StringBuilder, StringBuilder> errorMessageMapper) { | ||
| try { | ||
| FxAssert.verifyThat(false, alwaysFail(), errorMessageMapper); |
There was a problem hiding this comment.
Can we static import verifyThat here?
| * value is three spaces (e.g. {@code " "}). | ||
| * </p> | ||
| */ | ||
| public final class DebugUtils { |
There was a problem hiding this comment.
Should we maybe say "are there to specify what text is inserted for spacing purposes. This defaults to blah blah". What I mean is instead of explicitly calling it "tab", maybe rename to spacing?
There was a problem hiding this comment.
Perhaps a better name would be "indent"?
"Tab" is good in that that is what it's used for, indenting stuff. However, it's bad in that one may assume one needs to pass in "\t". "Spacing" isn't good either, though it fixes the problem with "tab," in that it doesn't communicate the indenting nature of it all. What are we spacing? Space between the current added text and the next one?
JordanMartinez
force-pushed
the
extractDebugUtils
branch
from
ae50160
to
28a8a66
Compare
JordanMartinez
force-pushed
the
extractDebugUtils
branch
from
28a8a66
to
1fbc521
Compare
|
Awesome. Thanks @JordanMartinez |
Resolves #420