On Wed, 24 Aug 2022 14:49:47 GMT, Jeanette Winzenburg <faste...@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 addressed feedback from @kleopatra : - implemented direct tests: TreeTableRowTest, TableRowTest - properly using StageLoader - tree table tests fail with the current master, pass with this PR. also, tree/table behavior is ok with a standalone tester (not checked in) Lessons learned: - StageLoader is required in this case to properly initialize the cells. No more calls to VirtualFlowTestUtils I do want to keep my TreeAndTableViewTest as it exercises different paths, which is beneficial. There will be more test cases applicable to both Tree- and TableView. thank you. ------------- PR: https://git.openjdk.org/jfx/pull/875
