Skip to content
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

Merged
merged 10 commits into from
May 18, 2020
Merged

Conversation

GeorchW
Copy link
Contributor

@GeorchW GeorchW commented May 14, 2020

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
Copy link
Contributor Author

GeorchW commented May 15, 2020

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
Collaborator

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

@GeorchW
Copy link
Contributor Author

GeorchW commented May 17, 2020

Sure.

@GeorchW
Copy link
Contributor Author

GeorchW commented May 17, 2020

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
Copy link
Contributor Author

GeorchW commented May 17, 2020

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
Copy link
Contributor Author

GeorchW commented May 17, 2020

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);
Copy link
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
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
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
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