Skip to content

Fix "forever spinning" (#211) - #252

Merged
stefanforsberg merged 10 commits into
formulahendry:masterfrom
GeorchW:no-watch
May 18, 2020
Merged

Fix "forever spinning" (#211)#252
stefanforsberg merged 10 commits into
formulahendry:masterfrom
GeorchW:no-watch

Conversation

@GeorchW

@GeorchW GeorchW commented May 14, 2020

Copy link
Copy Markdown
Contributor

There used to be an useless and complicated procedure based on file watching to detect new results files, which was error-prone and led to race conditions.

dotnet test writes the results files right before exiting, so we don't need any file watching; we can just wait for the process to terminate and then read the results. This simplifies the code a bit and removes the dependency on chokinar.

Fixes #211.

GeorchW added 4 commits May 14, 2020 17:55
Since the last commit moved the parsing to after the process ended,
we don't need to watch any files any more.
@GeorchW

GeorchW commented May 15, 2020

Copy link
Copy Markdown
Contributor Author

Uhm, Travis is failing the tests on Linux only, without further output, and it works on my machine. What's going on there?

(Same issue with my other PR, #253 .)

@stefanforsberg

Copy link
Copy Markdown
Collaborator

Hey @GeorchW. Would you mind merging the latest from master to this PR and I will take a look at it.

@GeorchW

GeorchW commented May 17, 2020

Copy link
Copy Markdown
Contributor Author

Sure.

@GeorchW

GeorchW commented May 17, 2020

Copy link
Copy Markdown
Contributor Author

Oops, I've realized that this PR breaks the Watch functionality. I didn't realize that it existed, since I've just glanced over the README and over-looked the fact that it's only activatable through a setting. Could fix this, though.

Notes to myself on how to fix it:

  • Move code to delete the results file out of the parsing logic
  • Only delete results files after the responsible process terminated, including the watch process, to avoid exceptions being thrown by accessing non-released file handles
  • Activate file watching again, but only for dotnet watch

@GeorchW

GeorchW commented May 17, 2020

Copy link
Copy Markdown
Contributor Author

Alternatively, since we're already parsing the dotnet watch test output, we can just additionally look for the string Results File: ..., and then start parsing that file. I think I'll do that.

GeorchW added 2 commits May 17, 2020 11:09
Removed the class "TestResultsFile" in the process, since it contained
only a single method and no state; made the method a free function
instead.

Also fixed a bug related to the parsing of the `dotnet watch` process
output - since the callback may not return whole lines, we have to
split and join them into lines by ourselves.

The method `TestCommands.watchRunningTests` was also removed to avoid
confusion about it; it did not actually watch any running tests.
@GeorchW

GeorchW commented May 17, 2020

Copy link
Copy Markdown
Contributor Author

Okay, I've done the latter now; I think it's a much cleaner solution. See also the comments on the respective commit.

Comment thread src/watch.ts
if (hadNewline) {
const line = startedLine.join("");
startedLine = [];
lines.push(line);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I test ran on my machine the lines seems to contain some whitespace which causes the equals check on line 77 to never become true. I guess this might be different on different OS (I'm on windows) but if I add a trim here bofre adding line to lines array it works better. Thoughts from you on this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I know that the dotnet watch command feeds the strings "watch : ", "Started.\n" into stdout on my machine. If we were to trim them, we would get something like "watch :Started", which would not match further down.

Could you give me some samples on which strings are being fed into buf here on your machine?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I just realized you meant the classical CRLF line ending issue - line endings are '\n' on linux, '\r' on Mac and "\r\n" on Windows. We should split on both, to avoid running into the same issue on Mac. I'll write some code and push it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test keeps spinning after test run

2 participants