sialcasa/mvvmFX

NotificationTestHelper should not start JavaFX thread

nils-christian opened this issue · 6 comments

nils-christian commented

Hi,

I am using the NotificationTestHelper as described in the Wiki for a unit test of the view model. I was surprised to learn that the NotificationTestHelper would try to start the JavaFX main thread. This means that one has an issue when trying to start the unit test in an environment without graphical device:

Graphics Device initialization failed for :  es2, sw
Error initializing QuantumRenderer: no suitable pipeline found
java.lang.RuntimeException: java.lang.RuntimeException: Error initializing QuantumRenderer: no suitable pipeline found
	at com.sun.javafx.tk.quantum.QuantumRenderer.getInstance(QuantumRenderer.java:280)
	at com.sun.javafx.tk.quantum.QuantumToolkit.init(QuantumToolkit.java:221)
	at com.sun.javafx.tk.Toolkit.getToolkit(Toolkit.java:248)
	at com.sun.javafx.application.PlatformImpl.startup(PlatformImpl.java:209)
	at javafx.embed.swing.JFXPanel.initFx(JFXPanel.java:249)
	at javafx.embed.swing.JFXPanel.<init>(JFXPanel.java:264)
	at de.saxsys.mvvmfx.utils.notifications.NotificationTestHelper.<init>(NotificationTestHelper.java:96)

Although I understand that the JavaFX thread has to be initialized in certain test cases, I would like to have a simple alternative without a whole JavaFX thread behind this. Especially when one wants to write a unit test for a simple view model, this should not be necessary. Please think about providing an alternative non-JavaFX-implementation of this helper class.

Thanks and best regards,

Nils

manuel-mauky commented

Hi,
the NotificationTestHelper needs a JavaFX thread because the notification mechanism itself has code that checks the thread on which messages are received. The goal is that in a View you can always subscribe to notification without having to worry whether the message will arrive in the JavaFX thread or not. You can send notifications from any thread and they will always be delivered on the JavaFX thread.

This behavior is handy in many situations but it can make testing harder. For this reason we have made the NotificationTestHelper aware of the JavaFX thread so that testing these cases is easier. Even if your message is send from any other thread in your ViewModel, you can still test it with the test-helper.

However, the trade-off is that you need a JavaFX thread in your unit test.
I hope this gives you the context of why the test-helper spawns a JavaFX Thread.

Now to the solution. In my experience using a JavaFX thread in test is not a big problem most of the time (which was the reason for the considerations I've discussed above). For example the mvvmFX code itself is tested in a headless CI environment on travic-ci. In our config file we use this code to start a simulated ui environment which makes using JavaFX Thread possible on linux servers:

- export DISPLAY=:99.0
- sh -e /etc/init.d/xvfb start

Maybe this can already help you. If this is not a sufficient solution for you I can also think of adjusting the NotificationTestHelper so that using the JavaFX thread can be disabled. However, the default behavior will still be the usage of a JavaFX thread. This is also needed because we try to avoid breaking changes.
We could add an optional constructor parameter of type boolean which can be used to disable all thread-related code. What do you think?

nils-christian commented

Hi,

I understand. However, as having a simulated UI is not possible in each environment, I would prefer if there would be some kind of headless version (also we don't need the overhead of JavaFX in a simple unit test for a view model in my opinion). I don't think it is enough to adapt the NotificationTestHelper though. The publish method of the ViewModel relies (by default) on the DefaultNotificationCenter which itself would again start a JavaFX thread as it seems. So, maybe it would be necessary to perform two steps for a headless component test:

  • Replace the NotificationCenter with a ThreadlessTestNotificationCenter which would just send each published event to the subscribers (or maybe with a single non-JavaFX-thread).
  • Add a new ThreadlessNotificationTestHelper (or with whatever name), which would simply collect to incoming events without the need of JavaFX. I would prefer a new component over adding more or less unrelated code to the original NotificationTestHelper.

If you are interested as well, I would dig into this and - if it works - provide a pull request.

Best regards

Nils

manuel-mauky commented

which itself would again start a JavaFX thread as it seems.

This is not the case. The DefaultNotificationCenter makes sure that if a JavaFX platform thread is available, then all messages send via a ViewModel are received on the JavaFX platform thread.
The NotificationCenter does not spawn a new JavaFX thread by itself.

If no JavaFX platform thread is available then the message is send directly on the default thread. This behavior is implemented here and there is a unit test that verifies that you can use the ViewModel Notifications in tests without a JavaFX Thread.

For notifications send outside of ViewModels directly with the NotificationCenter, no special thread handling is done and therefore in tests no JavaFX thread is needed. There is no special unit test available that verifies this but I've tested it out just yet to be sure.

The only problem is the NotificationTestHelper. It spawns a new JavaFX thread. And therefore I think we don't need any changes in the actual notification center but only in the test helper.

I agree that a separate test helper might be a good idea. ThreadlessNotificationTestHelper sounds ok for me. If you like to work this out I'm happy to accept a Pull-Request.
Please make sure to add a unit-test that shows what doesn't work with the existing test helper so that one can see the motivation for the new one. Also please adjust the java doc so that both classes reference each other with hints when to use one or the other.
Thanks for your contribution.

nils-christian commented

Hi,

Yes, I was mistaken. The DefaultNotificationCenter does not try to start the JavaFX thread. However, in a headless environment (I just used a simple docker to confirm this), the code still crashes, as the isFxApplicationThread method throws a runtime exception, if there is no toolkit available:

java.lang.RuntimeException: No toolkit found
	at com.sun.javafx.tk.Toolkit.getToolkit(Toolkit.java:260)
	at com.sun.javafx.application.PlatformImpl.isFxApplicationThread(PlatformImpl.java:264)
	at javafx.application.Platform.isFxApplicationThread(Platform.java:99)
	at de.saxsys.mvvmfx.utils.notifications.DefaultNotificationCenter.publish(DefaultNotificationCenter.java:76)

So it would still be necessary either to catch this exception (which is not a perfect solution, as it is a very generic runtime exception, which could also indicate a lot of other things) or to implement another notification center. Which solution would you prefer?

Best regards

Nils

manuel-mauky commented

I wasn't aware of this. This looks like a bug to me. The DefaultNotificationCenter should work on a headless environment.
At the moment the method looks like this:

public void publish(Object channel, String messageName, Object[] payload) {
	if (channelObserverMap.containsKey(channel)) {
		final ObserverMap observerMap = channelObserverMap.get(channel);

		if (Platform.isFxApplicationThread()) {
			publish(messageName, payload, observerMap);
		} else {
			try {
				Platform.runLater(() -> publish(messageName, payload, observerMap));
			} catch (IllegalStateException e) {

				// If the toolkit isn't initialized yet we will publish the notification directly.
				// In most cases this means that we are in a unit test and not JavaFX application is running.
				if (e.getMessage().equals("Toolkit not initialized")) {
					publish(messageName, payload, observerMap);
				} else {
					throw e;
				}
			}
		}
	}
}

We are already catching an exception for "Toolkit not initialized" and in a similar way we should catch the "No toolkit found" exception too.

Catching Exceptions like this is relatively slow compared to normal code flow. However, both situations can't happen at runtime in a real JavaFX application but only in testing environments. So from this perspective I think catching and handling the exceptions like this is OK.

EDIT:
Is there any possible way to unit-test your exception in a normal desktop environment? I assume this can only be tested on an isolated environment like your docker container?

nils-christian commented

Hi,

I added a pull request for the issue. I am not sure whether there is a possibility to test this in a normal desktop environment. I am at least not aware how to do this at the moment.

Best regards

Nils