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

[Andy Goryachev]

On Tue, 23 Aug 2022 10:39:24 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> Andy Goryachev has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - 8292353: typo
>>  - 8292353: typo
>
> Fix looks good (left a minor inline comment which you might follow or not :)
> 
> The tests, though, ... need some love:
> 
> As a general rule, it's preferred to assert a single state per test method instead of changing the state repeatedly inside the same method. We can see that something is wrong when having code comments like: 
> 
>     line 240:         assertFalse(row.isSelected()); // JDK-8292353 failure
> 
>     line 249:         assertFalse(row.isSelected()); // JDK-8292353 failure
> 
> running the test before the fix and the second failure is never reached. We _want_ to see the failures, all of them :)
> 
> Write a test as near to the cause as possible: here the cause is an incorrect selection state of the TreeTableRow under certain conditions. That's unrelated to any user interaction, so a setup faking such an interaction is a detour which might (or not) bring in their own trouble (aside: I do see the css parser warnings when running TreeAndTableViewTest inside Eclipse - that is [JDK-8291853](https://bugs.openjdk.org/browse/JDK-8291853) which we don't yet understand, except that something fishy might be going on ;) Best to use the most simple setup to test that exact state, here that would be an isolated Tree/TableRow thats configured with a Tree/TableView at some index and setup the view's selection state as needed, for examples see TreeTableRowTest or something like the following
> 
>     @Test
>     public void testRowSelectionStateOnCellSelectionEnabledMultiple() {
>         // configure view
>         addColumns();
>         tree.getSelectionModel().setCellSelectionEnabled(true);
>         tree.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
>         // configure tableRow        
>         int selected = 1;
>         cell.updateTreeTableView(tree);
>         cell.updateIndex(selected);
> 
>         // select all columns if possible
>         tree.getSelectionModel().select(selected, null);
>         assertEquals("sanity: all columns selected", tree.getColumns().size(),
>                 tree.getSelectionModel().getSelectedCells().size());
>         assertFalse("row must not be selected in cell selection mode", cell.isSelected());
>     }
> 
> and one for the second failure before the fix plus (for coverage) the other combinations of cell selection enablement/selection mode :) These should reside in TreeTableRowTest, IMO, to be "near" the other row state tests.
> 
> For coverage, we should also have the corresponding tests for TableRow (also isolated, not wrapped into faked user interaction). They should reside in TableRowTest. Which we don't have yet, time to add it :)
> 
> My suggestions: 
> 
> - convert the monolithic test methods into separate tests, each without changing the state you want to test
> - configure and test the selection state of an isolated row (without faking user interaction, similar to the example above)
> - for TreeTableRow, add the tests to TreeTableRowTest
> - for TableRow, add TableRowTest and add analogue methods as those you added for TreeTableRow
> - remove TreeAndTableViewTest

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?

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

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