-
Notifications
You must be signed in to change notification settings - Fork 78
A few functional/E2E tests that cover installation too #53
Conversation
@@ -200,7 +190,7 @@ private static string ResolveRootDirectory(string projectPath) | |||
|
|||
while (di.Parent != null) | |||
{ | |||
var globalJsonPath = Path.Combine(di.FullName, "global.json"); | |||
var globalJsonPath = Path.Combine(di.FullName, "project.json"); |
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.
Change variable name?
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.
Actually the whole function should be renamed to ResolveProjectDirectory
since we are only looking for the root of the project instead of the root of the solution.
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.
Fixed
27f2cbe
to
7262063
Compare
This test exercise became a big rewrite and I found and fixed a bunch of other bugs and limitations. Full list of bug fixes:
|
@@ -14,7 +14,7 @@ public Project(ProjectModel.Project runtimeProject) | |||
{ | |||
ProjectFile = runtimeProject.ProjectFilePath; | |||
ProjectDirectory = runtimeProject.ProjectDirectory; | |||
|
|||
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.
fix
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.
Is that the best you can do?
Unless there are more comments, I'm going to assume that this looks good and I'll merge it |
{ | ||
FileName = fileName, | ||
Arguments = arguments, | ||
RedirectStandardOutput = true |
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.
Don't you need to set UseShellExecute
to false to enable 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.
👍
var childs = GetAllChildIdsUnix(process, timeout); | ||
foreach (var childId in childs) | ||
{ | ||
KillProcessUnix(childId, timeout); |
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.
Do you kill the original process? Seems like you are only killing descendents of the original process.
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.
Good catch!
fc9f233
to
da72c06
Compare
Updated |
Looks good, |
- File watcher that takes the globbing patterns into account - A big rewrite of the core algorithm
da72c06
to
c43c37e
Compare
3 tests (1 disable) for dotnet watch + a mini scenario testing too.
@moozzyk maybe you want to use the scenario tester to test the iis integration tool too. It workarounds the fact that you have sources for the tool in the same repo.
Please review: @BrennanConroy @JunTaoLuo