-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Prevent EditAndContinueFeedbackDiagnosticFileProvider
from throwing an exception in Razor scenarios
#68619
Prevent EditAndContinueFeedbackDiagnosticFileProvider
from throwing an exception in Razor scenarios
#68619
Conversation
@@ -65,6 +65,12 @@ internal sealed class EditAndContinueFeedbackDiagnosticFileProvider : IFeedbackD | |||
var vsFeedbackTempDir = Path.Combine(_tempDir, VSFeedbackSemaphoreDir); | |||
_vsFeedbackSemaphoreFullPath = Path.Combine(vsFeedbackTempDir, VSFeedbackSemaphoreFileName); | |||
|
|||
// Directory may not exist in scenarios such as Razor integration tests | |||
if (!Directory.Exists(vsFeedbackTempDir)) |
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.
File watcher throws when the directory does not exist? What is the exact exception trace?
That'd be unfortunate since the intent was to detect when the file in that directory is created. The directory indeed might not exist at this time because VS Feedback didn't created it yet.
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.
Integration tests are struggling to run locally on my machine right now (trying to figure it out) so I don't have the exact stack trace at the moment, but the exception was:
ArgumentException: The directory name C:\Users\allichou\AppData\Local\Temp\Microsoft\VSFeedbackCollector is invalid.
and it was definitely originating from trying to initialize EditAndContinueFeedbackDiagnosticFileProvider
. Looking at the constructor code, it seems the most likely culprit is from FileSystemWatcher, as the documentation for the FileSystemWatcher constructor says it can throw:
// T:System.ArgumentException:
// The path parameter is an empty string (""). -or- The path specified through the
// path parameter does not exist.
//
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.
I see. ok.
Thanks for the review @tmat! To close out the convo, I was finally able to get integration tests running again, and the below was the more detailed exception:
|
This line in Razor's integration test logic throws an exception since one of the found providers is
EditAndContinueFeedbackDiagnosticFileProvider
, which seems to only account for Roslyn scenarios. This PR mitigates the issue by ensuring that we don't initialize aFileSystemWatcher
(which threw the original exception) unless the provided directory is valid.