On Tue, 23 Aug 2022 19:58:24 GMT, Andy Goryachev <[email protected]> wrote:
>
> I do agree that, in general, it might be beneficial to have one test case per
> condition or failure, but it should not be a requirement.
well, this project follows (should, at least, there are legacy areas ;)
established best practices for unit testing. If you want that changed, the best
place to discuss such a change in strategy is the mailinglist.
> I believe it is perfectly fine to have a test that executes a complex
> scenario.
we certainly can have tests for complex scenarios - but only if testing such a
complex scenario. That's not the case here.
>
> I do disagree with the requirement to write a highly artificial tests like
> testRowSelectionStateOnCellSelectionEnabledMultiple() because it makes too
> many assumptions on the internal structure of the code being tested.
with all due respect: that sentence is completely wrong.
- there is nothing artificial to test a class in the most minimal context
possible, in fact that's the whole point of unit testing
- the amount of assumptions on internal structure is zero, they use public api
to follow the three A (Arrange: create the cell, configure it at a fixed
location inside a virtualized control) A (Act: change the selection of the
control) A (Assert: verify the cell has updated its own state accordingly)
> After all, we have all these tests and yet plenty of bugs avoided detection
> for years.
that's mostly because the coverage is .. suboptimal ;) As this issue
demonstrates: there are no simple tests to guarantee a treeTableRow updates
itself reliably.
> I would rather have a test that simulates as much as possible the actual
> user interaction.
grabbing a tableRow from a temporary scenegraph (note: its scene is null once
you get it), adding its tableCells to a different scenegraph (note: after
mouseFirer constructor tableCell.getParent() != tableRow) and fire a mouseEvent
onto that isolated tableCell is _near .. actual user interaction_? That's not
case ;)
> Unless there is a very good reason
The very good reason is that your test setup is suboptimal (apart from changing
the recommended Attach-Act-Assert into
Attach-Act-Assert-ActAgain-AssertAgain-... ) for the issue here ;)
We have a very clear functional path to test: a fully configured TableRow must
sync its own selection state to the TableView's selection state at the location
of the row.
So all we need is a TableView (with columns, as we want cell selection) and a
TableRow configured to represent a location in that TableView. Changing table's
selection at the location of the row must change (or not, depending on
selection context) the selection state of the row. The most simple means to
change table's selection is programatically using public api on the table's
selectionModel with a direct mapping of
selectionModel.select -> tableRow.updateSelection
With yours:
stageloaderA -> createAndConfigure tableRows/Cells
-> tableRow = lookup row at index
-> stageloaderA.dispose
// at this point we are already far from any"real" context - no user
interaction possible
tableCell = lookup column for tableColumn
// nevertheless with rather normal micro-cosmos:
assertEquals(tableCell.getParent(), tableCell.getTableRow());
mouseFirer -> stageLoaderB -> insert targetNode into its own root
// at this point we have an isolated tableCell in a slightly weird state,
now failing the above assert:
assertEquals(tableCell.getParent(), tableCell.getTableRow());
// now fire a mouseEvent onto that isolated tableCell
// (which nevertheless is fully configured with its tableView and
location)
mouseFirer.doFirer -> tableCell.behaviour ->
__selectionModel.select -> tableRow.updateSelection__
and here at the very end we are again testing the same as in the simple case,
after going on a detour which might (or not) have side-effects or bugs of their
own. We might go that tour if we are specifically interested in that path, that
is when we are really want to test the behaviour (mouse and keyboard input) -
and then we must be certain that this very last expectation is fulfilled.
>
> What do you think?
Same as yesterday:
- missing direct tests
- unrelated, overly complex (and potentially testing the wrong thingy)
TreeAndTableViewTest
-------------
PR: https://git.openjdk.org/jfx/pull/875
