Community
Participate
Working Groups
The current databinding conformance tests fall into two categories: Observable*ContractTest and MutableObservable*ContractTest. This is inadequate in some cases (see Bug 212518 comment 15). An observable has two aspects that affect how thoroughly it should be tested: mutability (whether the observable can change) and modifiability (whether the observable can be changed by making API calls into it). Modifiability implies mutability, so there are only three possible combinations: Immutable, unmodifiable: ConstantValue (bug 212518), EmptyObservable(List|Set) Mutable, unmodifiable: ComputedValue, ComputedList, StaticObservable(List|Set) Mutable, modifiable: Writable(Value|List|Set|Map), JavaBeanObservableList All the conformance tests assume mutability, which forces implementors of immutable observables to subclass Observable*ContractTest (see attachment 85334 [details]) and override all the test methods which assume the observable can be changed. The lack of tests for immutable observables means that many of the classes in core databinding cannot be tested without the subclass hack. I propose that the contract tests be refactored to model the progression above as follows: ObservableValueContractTest (for any observable) MutableObservableValueContractTest (for mutable observables, modifiable or not) ModifiableObservableValueContractTest (for mutable, modifiable observables) I further suggest we do something to avoid having to add multiple conformance tests to the suite builder, e.g. instead of: new SuiteBuilder() .addObservableContractTest(MutableObservableValueContractTest.class, new Delegate()) .addObservableContractTest(ObservableValueContractTest.class, new Delegate()) .builder(); ..it could be simply: new SuiteBuilder() .addTest(MutableObservableValueContractTest.suite(new Delegate()) .build(); ..where MutableObservableValueContractTests.suite() implicitly adds the ObservableValueContractTest.suite() test suite, using the same delegate. I recognize that the first two class names suggested above have already been used, and this would change the meaning of existing code. However I still believe we should make the change and get this "right" before the M5 API freeze.
I've already refactored the tests according to the second scenario listed in comment #0, namely simplifying the construction of test suites. For example, in WritableTest, the following: public static Test suite() { Delegate delegate = new Delegate(); return new SuiteBuilder().addTests(WritableValueTest.class) .addObservableContractTest(ObservableValueContractTest.class, delegate).addObservableContractTest( MutableObservableValueContractTest.class, delegate) .build(); } is reduced to: public static Test suite() { TestSuite suite = new TestSuite("WritableValueTest"); suite.addTestSuite(WritableValueTest.class); suite.addTest(MutableObservableValueContractTest.suite(new Delegate())); return suite; } which to me is much more friendly. Notice that you don't need to add a ObservableValueContractTest.suite(), as this is implicit in MutableObservableValueContractTest.suite(). Unfortunately I can't submit a patch yet since these changes are tangled up with changes for other bugs..
Hi Matt. I'm a little rusty since I've been away but wanted to make sure that everything's as clear as mud. ;) (In reply to comment #0) > All the conformance tests assume mutability Technically they don't. They assume that some operation can occur that will fire a change event. The javadoc for IObservableContractDelegate states the following... "Invokes a change operation resulting in a change event being fired from the observable." The key is that a change event is to be fired, not that the value actually changed. It's a little ambiguous but I remember making this distinction when wording the javadoc. By doing this it allowed me to keep the API small since it was a first release. I wasn't sure how often this would be a concern, if it was worth added complexity to the API, and if developers would really understand the differences between all types. There are immutable implementations today that handled this without overriding test methods. The tests extend the implementation and expose the fireChange* method in most cases. An example of this is AbstractObservableListStub. This is done all over the place in the existing tests. I'm not saying a change shouldn't be made, just that the assumptions are a little different that was was previously stated... or for at least how I interpreted your interpretation. Good luck!
Looking back over the code, it appears that the only contract test that assumes mutability when it shouldn't is ObservableValueContractTest. So I may have originally blown this out of proportion. Nonetheless I still see benefits in refactoring the tests: * Make the class names more descriptive of what they test and what expectations are placed on the delegate. * The "static Test suite(Delegate)" usage is simpler and more concise than SuiteBuilder. * We should probably be testing unmodifiable observables to verify that UnsupportedOperationException is being thrown. * Hopefully it will eliminate the need for "stubclassing" the classes under test.
Created attachment 89374 [details] Preliminary refactoring MutableObservableValueContractTest renamed to ModifiableObservableValueContractTest ObservableValueContractTest renamed to MutableObservableValueContractTest Extracted ObservableValueContractTest as superclass of MutableObservableValueContractTest, with the applicable test methods. Added static suite(IObservableValueContractDelegate) suite methods, redirected all relevant existing SuiteBuilder code to the new suite(Delegate) method.
Created attachment 89375 [details] mylyn/context/zip
Note to self: calling ObservableTracker.getterCalled() is a waste of time in immutable observables (EmptyObservableList) so the tests which require getterCalled() should be moved up to the mutable observables suite.
Progress changes released to HEAD > 20080306: Added static suite(IObservableContractDelegate) methods to all observable contract test classes. Migrated existing tests to new API. The old API still works. Not done: Separating out tests according to immutable -> mutable -> modifiable progression Change test class names to make it more clear which one to use.
Please adjust the target milestone, so it does not point at a closed milestone in the past.
Removing milestone.
Still planned for 3.5?
Deferred to 3.6
In order to be able to run the MutableObservable*ContractTests on non-modifiable (but mutable) observables as well, we might also change the tests in such a way that the mutator methods are never invoked directly but instead they are redirected through the IObservable*ContractDelegate classes. That way, the delegate classes could do whatever is necessary in order to let the tests work on non-modifiable observables as well (for modifiable observables, they would simply invoke the mutator methods on the observable directly). For skipping individual tests which require non-supported operations on the observables, we might define a special exception which the delegates might throw for the unsupported operations. Whenever such an exception is thrown, the test can just be skipped without requiring clients to overwrite individual test methods in order to disable them.
Removed from 3.6. Owners can re-assess. PW
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
