Update ActionDebouncer.cs to support tests with no Sync context #15804
Conversation
| // The TaskScheduler.FromCurrentSynchronizationContext() exists only if there is a valid SyncronizationContex. | ||
| // Calling this method from a non UI thread could have a null SyncronizationContex.Current, | ||
| // so in that case we use the default TaskScheduler which uses the thread pool. | ||
| var taskScheduler = SynchronizationContext.Current != null ? TaskScheduler.FromCurrentSynchronizationContext() : TaskScheduler.Default; |
There was a problem hiding this comment.
This change makes it a bit harder to predict where the continuation will be executed. The alternative is to identify all tests that call into the ActionDebouncer and make sure they setup a SyncContext before (seemed too tedious to do)
There was a problem hiding this comment.
if the sync context is not on the UI thread can we log something?
There was a problem hiding this comment.
The sync context should always be setup in a UI thread. But I can add it if it makes it more understandable
UI Smoke TestsTest: success. 11 passed, 0 failed. |
|
@pinzart90 is this failure intermittent or repeatable? |
Locally it is repeatable. It also failed at least once on the build machines. |
|
One single known flaky test is failing |
| } | ||
| else | ||
| {// This might happen when running tests in non UI threads. | ||
| if (Dispatcher.CurrentDispatcher != null) |
There was a problem hiding this comment.
shouldn't this check for null
couldn't current dispatcher be any dispatcher - not just the UI dispatcher?
There was a problem hiding this comment.
Dispatcher.CurrentDispatcher != null - this means it is in the UI thread.
The check is like this:
IF the current sync context not null - then all is as expected.
If the current sync context is null :
- then we are probably in a unit test (non UI thread)
- but if we are in a UI thread, then this case is weird and we should log it
Does this make sense?
There was a problem hiding this comment.
How does Dispatcher.CurrentDispatcher != null ensure it's the UI thread?
I thought that this just accessed the dispatcher of the thread?
taken from SO:
Dispatcher.CurrentDispatcher gets the dispatcher for the current thread. So, if you're looking for the UI thread's Dispatcher from a background process, don't use this.
Application.Current.Dispatcher will always give you the UI thread's dispatcher, as this is the thread that spins up the sole Application instance.
There was a problem hiding this comment.
Yeah, I wasn't too sure about that. Looks like Application.Current?.Dispatcher != null would be a better check for a UI thread,
There was a problem hiding this comment.
We have a lot of tests that do not setup any SyncronizationContext but still test WPF controls.
This does not work when calling FromCurrentSynchronizationContext (throws exception)
Ex
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/17145/testReport/junit/DynamoCoreWpfTests/ConverterTests/SearchHighlightMarginConverterTest/
Purpose
(FILL ME IN) This section describes why this PR is here. Usually it would include a reference to the tracking task that it is part or all of the solution for.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of