-
Notifications
You must be signed in to change notification settings - Fork 997
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: Application.Restart throws InvalidOperationException #2845
Conversation
@@ -1167,7 +1167,7 @@ public static void Restart() | |||
sb.Append(arguments[arguments.Length - 1]); | |||
sb.Append('"'); | |||
} | |||
ProcessStartInfo currentStartInfo = Process.GetCurrentProcess().StartInfo; | |||
ProcessStartInfo currentStartInfo = new ProcessStartInfo(); |
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 there a test we can add?
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 don't think this method is unit-testable, there are too many moving parts here. Can't think of a relatively low-effort automation either, we haven't done any automated integration tests yet...
However I do agree we some form a verification that it is working as intended.
@EatonZ please have a look at #2801. I have created few PS scripts that generate net472 and netcoreapp5.0 app, and then ran manual tests against them to confirm the fix.
You could tweak the "single app" script and then run a series of tests with and without input parameters.
A verification would be something along the lines of:
# build and run the app, passing parameters to it
dotnet run -- a b c
# app restarted, confirm parameters
Get-WmiObject Win32_Process | where name -eq your_app_name.exe | select commandline
@hughbe any thoughts on how else we can test 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.
@RussKie It seems like we could use the scripts you made there, with 1 tweak: replace => System.Windows.Forms.Application.ExecutablePath;
with => System.Windows.Forms.Application.Restart();
, wrap that in a try/catch and report a failed test if it throws an exception.
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.
Yes. We still need to confirm all input arguments are being passed to the new instance.
Codecov Report
@@ Coverage Diff @@
## master #2845 +/- ##
===================================================
+ Coverage 59.52728% 59.53611% +0.00883%
===================================================
Files 1236 1236
Lines 429934 429934
Branches 38787 38787
===================================================
+ Hits 255928 255966 +38
+ Misses 168636 168590 -46
- Partials 5370 5378 +8
|
Sorry for the delay, it has slipped off my radar. |
Fixes #2769
Proposed changes
Implemented the suggested fix from @stephentoub.
Customer Impact
This fixes a method broken by changes in .NET Core. Most customers are probably unaware of the broken method since it is not a documented breaking change, nor are there other issues reporting the breakage.
Regression?
Risk
There should be no risk. I have tested this fix myself and confirmed that it works.
Microsoft Reviewers: Open in CodeFlow