Observable tests fail after adding subscribeOn EDT call to EventSources

[epb-644]

I ran into an issue when trying to implement the EventSource changes agreed upon for issue #3.

We agreed to make all EventSources implicitly subscribe on the Swing thread. This was to be accomplished by adding a subscribeOn(SwingScheduler.getInstance()) call to each source, as shown in @mikebaum's example below:

public static Observable<ItemEvent> fromItemEventsOf(final ItemSelectable itemSelectable) {
        return Observable.create(new OnSubscribe<ItemEvent>() {
            @Override
            public void call(final Subscriber<? super ItemEvent> subscriber) {
                final ItemListener listener = new ItemListener() {
                    @Override
                    public void itemStateChanged(ItemEvent event) {
                        subscriber.onNext(event);
                    }
                };
                itemSelectable.addItemListener(listener);
                subscriber.add(Subscriptions.create( new Action0() {
                    @Override
                    public void call() {
                        itemSelectable.removeItemListener(listener);
                    }
                }));
            }
        }).subscribeOn( SwingScheduler.getInstance() );
    }

Unfortunately, this subscribeOn call causes our tests to fail. In the code above, the OnSubscribe's call method gets executed after ItemEvents are fired in the unit test.

Since the test already executes on the EDT, subscribeOn( SwingScheduler.getInstance()) causes the SwingScheduler to put the subscription call to the end of the EDT's queue. Therefore events fire before the subscription is properly initialized.

I propose that we modify SwingScheduler's behavior when the calling thread is the EDT. I think we should not put the subscription in an invokeLater if the caller is already on the Swing thread. We should just run it immediately.

Any thoughts? This would change the behavior of nested actions in SwingScheduler, of which SwingSchedulerTest is very specific about.

Let me know if I'm ignoring anything.

[mikebaum]

@eddieburns55 , sorry for disappearing been super busy these last few weeks. Thanks for taking on the task to update the EventSources. I had started to make the changes myself and ran into exactly the same problem.

The approach I started to take was rather to split up the tests by adding a setup and tear down. I also added a helper method to SwingTestHelper called waitForEdt(). Sorry for the large comment, but here's how I updated AbstractButtonSourceTest:

@RunWith(MockitoJUnitRunner.class)
public class AbstractButtonSourceTest {
    @Mock private Action1<ActionEvent> action;
    @Mock private Action1<Throwable> error;
    @Mock private Action0 complete;
    private TestButton button;
    private Subscription sub;
    private ActionEvent event;

    @Before
    public void setup() throws Throwable {
        SwingTestHelper.create().runInEventDispatchThread(new Action0() {
            @Override
            public void call()
            {
                button = new TestButton();
                event = new ActionEvent(this, 1, "command");
            } 
        }).awaitTerminal();

        sub = AbstractButtonSource.fromActionOf(button).subscribe(action, error, complete);
        SwingTestHelper.waitForEDT();
        Assert.assertFalse(sub.isUnsubscribed());

        // sanity check
        verifyZeroInteractions(action, error, complete);
    }

    @After
    public void tearDown() throws Throwable {
        sub.unsubscribe();
        SwingTestHelper.waitForEDT();
        Assert.assertTrue(sub.isUnsubscribed());

        SwingTestHelper.create().runInEventDispatchThread(new Action0() {
        @Override
        public void call() {
        button.testAction(event);
        verifyNoMoreInteractions(action, error, complete);
        }
        }).awaitTerminal();
    }

    @Test
    public void testObservingActionEvents() throws Throwable {
        SwingTestHelper.create().runInEventDispatchThread(new Action0() {
            @Override
            public void call() {
                button.testAction(event);
                verify(action, times(1)).call(Matchers.<ActionEvent> any());

                button.testAction(event);
                verify(action, times(2)).call(Matchers.<ActionEvent> any());
            }

        }).awaitTerminal();
    }

    @SuppressWarnings("serial")
    private static class TestButton extends JButton
    {
        void testAction(ActionEvent event) {
            fireActionPerformed(event);
        }

        @Override
        public void addActionListener(ActionListener listener) {
            Assert.assertTrue(SwingUtilities.isEventDispatchThread());
            super.addActionListener(listener);
        }

        @Override
        public void removeActionListener(ActionListener listener) {
            Assert.assertTrue(SwingUtilities.isEventDispatchThread());
            super.removeActionListener(listener);
        }
    }
}

and here's SwingTestHelper.waitForEdt():

public static void waitForEDT() throws InterruptedException
{
    final CountDownLatch latch = new CountDownLatch(1);
    SwingUtilities.invokeLater(new Runnable() {
        @Override
        public void run() {
            latch.countDown(); 
        }
    });
    latch.await();        
 }

That said, I think I like your approach better, since it seems unnecessary to invokeLater if you're already on the EDT.

I suppose my only reservation to the invokeNowOrLater approach was that it could potentially re-order events if two events were received from different threads, one from the EDT and one from a background thread for example. Perhaps this reservation is ill founded since we really aught to always be using observeOn( SwingScheduler.getInstance() ).

FYI, we've started to use Rx in our GUI code at my work and two of my colleagues convinced me that when we subscribe or unsubscribe our listeners that we should blowup if we're not on the EDT. The reasoning was that not blowing up might be covering up a latent bug, so it's probably a good idea to expose it.

I think my issue with the original implementation of the FromEvent pattern was that it was not symmetrical, since we we're blowing up on subscribe, but on not on unsubscribe.

[epb-644]

Interesting solution for the tests. To my mind, though, the original behavior is flat out wrong. It's rare To call invokeLater in Swing when you're already on the EDT, so why do it here?

[mikebaum]

Yes, I agree, it would seems unnecessary to be calling invokeLater when on the EDT, so perhaps the SwingScheduler aught to be updated as you suggest. As long as this contract is clearly documented I think it should be fine.

[epb-644]

The PR is up. See pull request #17.

[epb-644]

Closed by pr #17