[gwt-contrib] Re: Adding some extra capabilities to CountingEventBus so that we can reduce the boilerplate involve... (issue1526803)
LGTM http://gwt-code-reviews.appspot.com/1526803/ -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding some extra capabilities to CountingEventBus so that we can reduce the boilerplate involve... (issue1526803)
http://gwt-code-reviews.appspot.com/1526803/diff/3/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
File
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
(right):
http://gwt-code-reviews.appspot.com/1526803/diff/3/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode47
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:47:
handlerCounts.increment(type);
I still think we should increment last since that's more likely to catch
failures. Any tests depending on the count being wrong (if there are
any) should be fixed.
http://gwt-code-reviews.appspot.com/1526803/diff/3/user/test/com/google/web/bindery/event/shared/testing/CountingEventBusTest.java
File
user/test/com/google/web/bindery/event/shared/testing/CountingEventBusTest.java
(right):
http://gwt-code-reviews.appspot.com/1526803/diff/3/user/test/com/google/web/bindery/event/shared/testing/CountingEventBusTest.java#newcode40
user/test/com/google/web/bindery/event/shared/testing/CountingEventBusTest.java:40:
assertEquals(1, eventBus.getHandlerCount(FooEvent.TYPE));
These numbers should always be the same, so to cut down on verbosity,
could you extract a two-line helper method to check both counts?
http://gwt-code-reviews.appspot.com/1526803/diff/3/user/test/com/google/web/bindery/event/shared/testing/CountingEventBusTest.java#newcode102
user/test/com/google/web/bindery/event/shared/testing/CountingEventBusTest.java:102:
assertEquals(0, eventBus.getFiredCountFromSource(FooEvent.TYPE,
source1));
perhaps extract checkSourceEvents() method to cut down on repetition.
http://gwt-code-reviews.appspot.com/1526803/diff/3/user/test/com/google/web/bindery/event/shared/testing/CountingEventBusTest.java#newcode144
user/test/com/google/web/bindery/event/shared/testing/CountingEventBusTest.java:144:
private void assertTotalEventsFired_NoSource(Type? type, int fired) {
Perhaps rename to something shorter like checkTotalEvents.
Also, I usually put the expected value first to match assertEquals().
http://gwt-code-reviews.appspot.com/1526803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Adding some extra capabilities to CountingEventBus so that we can reduce the boilerplate involve... (issue1526803)
http://gwt-code-reviews.appspot.com/1526803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
File
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java
(right):
http://gwt-code-reviews.appspot.com/1526803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode46
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:46:
public H HandlerRegistration addHandler(TypeH type, H handler) {
If we want to count only successful method calls, we should move the
increment() calls to after the wrapped method calls, so the counter
isn't incremented when the wrapped method throws an exception.
In particular, it looks like SimpleEvent bus requires that type is not
null. It might be a good idea to do a null check in this method, so we
don't have to read a lot of other code to figure that out.
http://gwt-code-reviews.appspot.com/1526803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode92
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:92:
* events may not have been passed to any handlers.
Should clarify whether we're only counting successful calls. Also, what
happens if a handler throws an exception?
(It might be nice to have a counter for errors, but I think that's a
separate CL.)
http://gwt-code-reviews.appspot.com/1526803/diff/1/user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java#newcode118
user/src/com/google/web/bindery/event/shared/testing/CountingEventBus.java:118:
TypeSourcePair(Type? type, Object source) {
It looks like type should not be null (at least in SimpleEventBus) but
source can be null. Maybe put a null check and a comment? I had to read
a fair bit of code to figure that out.
http://gwt-code-reviews.appspot.com/1526803/
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
