PlugValueWidgetTest : Fix handling of background updates (alternative approach) - #7082
Open
johnhaddon wants to merge 1 commit into
Open
Conversation
This fixes test failures that looked like this :
```
Traceback (most recent call last):
File "D:\a\gaffer\gaffer\build\python\GafferUITest\PlugValueWidgetTest.py", line 453, in testContextTrackerUpdates
self.waitForUpdate( widget )
File "D:\a\gaffer\gaffer\build\python\GafferUITest\PlugValueWidgetTest.py", line 62, in waitForUpdate
handler.assertCalled()
File "D:\a\gaffer\gaffer\build\python\GafferTest\ParallelAlgoTest.py", line 92, in assertCalled
self.receive( timeout )()
^^^^^^^^^^^^^^^^^^^^^^^
File "D:\a\gaffer\gaffer\build\python\GafferTest\ParallelAlgoTest.py", line 86, in receive
raise AssertionError( "UIThread call not made within {} seconds".format( timeout ) )
AssertionError: UIThread call not made within 30.0 seconds
```
Our old approach of calling `waitForUpdate()` after running the code to trigger
the update was bogus - it was possible for the update to have finished before we
even got to calling `waitForUpdate()`. By triggering the update from _inside_ the
UIThreadCallHandler's scope we avoid this possibility.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes the same test failures as #7081, but in a more conservative way, affecting only the tests which used
PlugValueWidgetTest.waitForUpdate(). The fundamental issue is how we deal with testing UI updates involving LazyMethod and BackgroundMethod, where in both cases we need to wait for the UI to update before we can make our assertions. We've taken two main approaches to that :1.
TestCase.waitForIdle(): This just runs the Qt event loop for a little bit and then stops. And calls toParallelAlgo::callOnUIThread()are serviced by the regular handler in EventLoop.py. This has benefits in that it's pretty simple, and you don't need to know how many UI thread calls you're expecting. But it's also pretty vague - it's not clear how much idling is enough before we can make our assertions, so we're always in danger of not idling enough.2.
ParallelAlgoTest.UIThreadCallHandler().assertCalled(): This is independent of the event loop. It installs its own UI thread call handler, and syncs that with calls toassertCalled(). It's very controlled, and always waits until the required updates are complete. But it's also tied quite closely to the implementation of the widgets we're testing, because we need to know how many UI thread calls to expect.Before, each test could decide what approach it wanted to take. But #7081 forces everything into option 2, meaning that tests that expect
waitForIdle()to execute UI thread calls will fail. It would be great to settle on one approach, but that's too much work right now. And maybe a different approach entirely would be better anyway - something like theassertEventually()we use for interactive render tests?Anyway, to summarise, I'm proposing that this PR replaces #7081.