-
Notifications
You must be signed in to change notification settings - Fork 98
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix "forever spinning" (#211) #252
Conversation
Since the last commit moved the parsing to after the process ended, we don't need to watch any files any more.
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 .) |
Hey @GeorchW. Would you mind merging the latest from master to this PR and I will take a look at it. |
Sure. |
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:
|
Alternatively, since we're already parsing the |
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.
Okay, I've done the latter now; I think it's a much cleaner solution. See also the comments on the respective commit. |
if (hadNewline) { | ||
const line = startedLine.join(""); | ||
startedLine = []; | ||
lines.push(line); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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.