Make AppVeyor tests pass #733
Conversation
This reverts commit 0bdbbe0.
| { | ||
| public static async Task<T> RunWithTimeout<T>(this Task<T> task, int timeoutMillis = 10000) | ||
| { | ||
| if (await Task.WhenAny(task, Task.Delay(timeoutMillis)) == task) |
There was a problem hiding this comment.
Might be a little easier to use the tasks' AsyncWaitHandle
if (!((IAsyncResult)task).AsyncWaitHandle.WaitOne(timeoutMillis))
{
throw new TimeoutException();
}
return await task;Don't know which route is better, just giving an alternate
There was a problem hiding this comment.
Ah! That definitely feels more "as designed" than my abuse of tasks
| @@ -90,7 +90,7 @@ public async Task ServiceReturnsSyntaxErrors() | |||
| string.IsNullOrEmpty(diagnostics.Diagnostics[0].Message)); | |||
| } | |||
|
|
|||
| [Fact(Skip = "Skipping until Script Analyzer integration is added back")] | |||
| @@ -292,7 +292,7 @@ public async Task FindsReferencesOfVariable() | |||
| Assert.Equal(12, locations[2].Range.Start.Character); | |||
| } | |||
|
|
|||
| [Fact] | |||
| [Fact(Skip = "AppVeyor is currently hanging on this test it seems")] | |||
There was a problem hiding this comment.
is this test the culprit?
There was a problem hiding this comment.
It seems to be. It's strange. Just skipping this test doesn't work, and just putting timeouts on the async tests doesn't work. But both together passes, consistently. Naturally, "if you fixed it but you don't know how, you didn't fix it"...
For now I just want to get tests running and passing in CI, but I think this is another signal that reworking the tests is needed
There was a problem hiding this comment.
I wonder if the timeout isn't doing anything for some tests because there isn't an await soon enough. From what I understand async methods are ran on the same thread they are started until they hit the first await keyword. There may be something that appears to be async but doesn't actually use the keyword enough.
If you want you could try forcing the test into another thread by running it with Task.Run
There was a problem hiding this comment.
It's back to hanging again 😠
There was a problem hiding this comment.
The initial invocation of the task needs to be in the Task.Run block. My guess is that the method never actually returns a task in the first place, it just hangs before it hits an await.
If you wanted to make it a utility method you could do something like
async Task<T> RunWithTimeout<T>(Func<Task<T>> test, int timeout)
{
((IAsyncResult)Task.Run(test)).WaitOne(..etcThen call it with
RunWithTimeout(async () => await MyTestName(), 1000);There was a problem hiding this comment.
Bugger, still hanging
There was a problem hiding this comment.
Ok, reverting back to the last commit that passed this passes again... 😒
There was a problem hiding this comment.
I'm just going to merge this for now so the other PRs can test properly
There was a problem hiding this comment.
async methods are ran on the same thread they are started until they hit the first await keyword.
Even then, IIRC an async operation can complete during the initial awaited async call if the operation determines it can return the result right away.
Change did not fix hanging tests
|
I've reverted this PR since it didn't actually make the tests pass. At this point I think trying to fix the hang is going to be a bad investment of effort. I'm already working on tests, so I may as well just do my best to rewrite these tests in a way that makes them work. |
The
FindNoReferencesOnEmptyLinetest is hanging in AppVeyor (not on a local Windows machine).This skips it.
We should work out why this is though, especially since it isn't even an
asynctest...I've also re-enabled a bunch of previously skipped tests, because they seem to be passing.
EDIT: Turns out I had to add a timeout to the task tests (even though none of them actually time out...)