#538 Using ControlsFxVisualizer causes node graph inconsistencies

[ntherrien]

Wrapped ValidationVisualizationBase in a Platform.runLater to ensure the node graph completes initialization before any Visualizers are created on top of it.

Added a test class.

Added a utility function that allows to pause the Platform thread in order to allow the test code to validate whether or not a function is using runLater. Pausing the thread is required since it is possible that the Runnable be executed before the test has had the time to validate.

[manuel-mauky]

Thanks for this Pull-Request. I've only found some minor issues with star-imports. I have to admit that we don't have coding guidelines for the project with which you would have had a chance to see this upfront.

[ntherrien]

How do I update this Pull Request with the requested review changes? I'm fairly new to github and pull requests.

[manuel-mauky]

Updating Pull-Requests is easy. Create a new commit with the changes on your branch. Then it should be added here in the Pull-Request automatically

[ntherrien]

Okay I did what you asked me to do but after I rebased the pull request changed files has changed. I don't know where they come from and am at loss.

Here's what i did:

1) I rebased my fork and pushed it 2) I rebased my branch locally 3) I made the fixes 4) I tried to push my branch but that failed due to previous pull request merge commit. 5) I pulled my branch to fix the conflicts locally 6) I pushed my branch successfully.

But now the diff shows changes from some other commits from rebasing. No idea how to get rid of that. They come from steps 5 and 6. None of my commits contain these changes except for the push.

And yet, GitHub says if we accepted this pull request today, there would be no conflicts with the base branch...

[manuel-mauky]

A rebase is not needed. Normally it would work this way:

1. checkout your branch from your fork
2. Make some changes and commit them
3. push the commit

That's all. No rebasing or merging is needed. Rebasing manipulates the history which results in merge conflicts and crazy effects like the one we see here. Durring rebase the hash-ids of commits are changed. Therefore git can't detect that some of the commits from other people are already exist in the target branch because they have different hashes. In general, rebasing is only save with commits only visible on your local machine.

Actually I don't really know how to repair this state. I will try to cherry-pick your good commits.

[manuel-mauky]

I've done a cherry-pick with your commit here. I hope I haven't missed anything

[manuel-mauky]

Now your tests don't work anymore.

I have to admit that I don't really get your new test util putPlatformThreadOnHold.

Given the following code:

cut.doSomethingOnPlatformThread();

Semaphone semaphore = new Semaphore(0);
FxTestingUtils.putPlatformThreadOnHold(semaphore);
Mockito.verifyZeroInteractions(testDummy);

How do you prevent that the callback in doSomethingOnPlatformThread is executed before you invoke putPlatformThreadOnHold()? If JavaFX decides to immediatly execute the callback even before you got the chance to invoke the new util function then the Mockito.verify statement is wrong. It is possible that the interaction with the testDummy is already done at this point in time and as far as I can see, there is no way to prevent this. Can you please explain in a little more detail how this is intended to work?

[ntherrien]

putPlatformThreadOnHold() is introducing a special runnable in the thread's execution queue. That runnable will only complete once the semaphore is released.

In this test, the queue is paired to a single threaded executor and organized in the following way: [ runnable1: semaphore, runnable2: code under test ]

We need the semaphore blocking the queue precisely because of the situation your mentioned: it is possible that JavaFX decides to execute the code directly.

Using this setup, it does not matter which decision JavaFX will make (whether or not to execute right away). In both cases, the semaphore will block and this gives us time to do verifications before we let the execution go through by unlocking the semaphore.

[ntherrien]

About the merging, I couldn't update my pull request because it was out of date with the upstream develop branch (the one this project). I know I probably don't understand how this works yet, but the errors I was getting were instructing me to update my branch so that it was in sync with the latest develop branch.

When I have more time, I will delete the branch and start over if that's easier.

[ntherrien]

I looked into the build output from your link: Failed tests: ValidationVisualizerBaseTest.testDelayedInitialization:84 No interactions wanted here: -> at de.saxsys.mvvmfx.utils.validation.visualization.ValidationVisualizerBaseTest.testDelayedInitialization(ValidationVisualizerBaseTest.java:84) But found this interaction on mock 'testDummy': -> at de.saxsys.mvvmfx.utils.validation.visualization.ValidationVisualizerBaseTest$1.applyRequiredVisualization(ValidationVisualizerBaseTest.java:57)

Maybe the semaphore logic in the test is flawed after all. Should I just remove the test? I don't see how else we could test this class!!!