On Tue, 27 Apr 2021 10:18:24 GMT, Jeanette Winzenburg <faste...@openjdk.org> 
wrote:

> A couple of notes to your test - my personal preferences:
> 
> * they should cover all changes, that is test both Tree- and TreeTableView
> * if possible, have a test that really cover the report: here that would be 
> throwing a AIOOP on expanding again (with a filter to re-select all children)
> * have a test for underlying reason of the reported misbehavior: here the 
> incorrect change
> * testing the change should be complete (in testing its state)
> 
> I have seen you using the MockListObserver in an earlier version of this PR - 
> which is perfect for the last bullet, we should use it always in asserting 
> the correctness of listChange notification :) The only very minor drawback is 
> that it requires to update the Eclipse classpath to add-exports for the base 
> testing package (not a big deal).

I think you're right that the same test could also be added for TreeTableView, 
and that the test should probably cover the entirety of the change 
notification. However, my preference is to not create a test that duplicates 
all aspects of the specific program that was attached to the JBS issue. I think 
it's more meaningful to test the most minimal setup that can reproduce the 
underlying issue; adding bells and whistles (as was done in the JBS code 
sample) feels like adding unnecessary noise to the test.

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

PR: https://git.openjdk.java.net/jfx/pull/480

Reply via email to