RFR: 8292353: TableRow vs. TreeTableRow: inconsistent visuals in cell selection mode [v6]

[Jeanette Winzenburg]

On Tue, 23 Aug 2022 19:58:24 GMT, Andy Goryachev <angorya at openjdk.org> 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