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

[Andy Goryachev]

On Wed, 24 Aug 2022 14:49:47 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> Dear @kleopatra : thank you for insightful comments and suggestions!
>> 
>> 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.  I believe it is perfectly fine to have a test that executes a complex scenario.  In this particular case, the failure is known, it is described in JDK-8292353, and the fact that there are multiple places where test fails (before the fix) is not important.  What is important is that the failing paths are exercised and pass as a result of the fix.
>> 
>> 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.  I would rather have a test that simulates as much as possible the actual user interaction.  I am not saying we have to forbid simple tests, but rather we need both.  After all, we have all these tests and yet plenty of bugs avoided detection for years.  It took me 10 minutes to write and test a simple example and identify a number of issues (though many of them already in JBS) with Tree/TableView.
>> 
>> I'd say let's allow some diversity in our testing approaches.  Unless there is a *very* good reason to refactor, I'd rather leave TreeAndTableViewTest as is and not touch other files, as they are too big and I don't want to replicate code.
>> 
>> What do you think?
>
>> 
>> 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

Dear @kleopatra : 

Thank you so much for the wisdom.  Let me try to refactor the test cases.

A few questions:

1. StageLoader - since it is being disposed of by MouseFirer, would it make sense to explain how to use it properly in its javadoc?  Perhaps a StageLoader needs to be created and explicitly disposed of at the beginning and end of each test?

2. I noticed that despite MouseFirer's firing MOUSE_PRESSED followed by MOUSE_RELEASED, the cell's pseudostyles contain "pressed" which seems to be incorrect.  Perhaps it is due to multiple stages being created?

3. Why do I need to call VirtualFlowTestUtils.getCell(table, 0) for the cells to be created?  Do you know what is missing here? 

thank you.

-------------

PR: https://git.openjdk.org/jfx/pull/875