Summary: | [DataBinding] Refactor conformance tests for mutability vs modifiability aspects | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | [Eclipse Project] Platform | Reporter: | Matthew Hall <qualidafial> | ||||||
Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||||
Status: | ASSIGNED --- | QA Contact: | |||||||
Severity: | enhancement | ||||||||
Priority: | P3 | CC: | bokowski, bradleyjames, mallo.ovidio | ||||||
Version: | 3.4 | ||||||||
Target Milestone: | --- | ||||||||
Hardware: | PC | ||||||||
OS: | All | ||||||||
Whiteboard: | |||||||||
Attachments: |
|
Description
Matthew Hall
2007-12-17 02:25:57 EST
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. |