Use CPS for tests where saving documents needs to be reliable #70983
Conversation
dotnet-issue-labeler
bot
added
Area-Infrastructure
untriaged
|
When will this be ready to review? 🙂 |
The csproj project system does not correctly detect unsaved files on build. See dotnet#70938.
|
@Cosifne it should be ready now |
| if (s_editorPackage is null) | ||
| return; | ||
|
|
||
| var folder = s_editorPackage.UserLocalDataPath; |
There was a problem hiding this comment.
Why are we asking the editor package for this versus just calling one of the other APIs for this same path?
There was a problem hiding this comment.
Would love to learn about APIs that might exist for getting the right path here. I wrote a rather brute force approach in Razor to do it, which I've always wondered when it might break: https://github.com/dotnet/razor/blob/main/src/Razor/test/Microsoft.VisualStudio.Razor.IntegrationTests/VisualStudioLogging.cs#L134
There was a problem hiding this comment.
➡️ I now obtain the path from IVsComponentModelHost
| } | ||
| catch (Exception ex) | ||
| { | ||
| content = ex.ToString(); |
There was a problem hiding this comment.
| content = ex.ToString(); | |
| content = $"Exception thrown while reading {file}: {ex}"; |
So it's more clear this the content of the file in that case.
| namespace Roslyn.VisualStudio.NewIntegrationTests.InProcess; | ||
|
|
||
| [TestService] | ||
| internal partial class ScreenshotInProcess |
There was a problem hiding this comment.
You might want to add a one-or-two sentence comment here just to say "this takes screenshots and saves them as an .apng" and link to the spec. Mostly because somebody who sees this code who doesn't know what did here first would go "what is this???"
| DataCollectionService.RegisterCustomLogger( | ||
| static fullPath => | ||
| { | ||
| lock (s_frames) |
There was a problem hiding this comment.
Add a comment to s_frames that we're taking locks this way. (No concerns that we are, but is still good to make it clear.)
| if (s_frames.Count == 0) | ||
| return; |
There was a problem hiding this comment.
Given we're going to add a frame at the end, why skip this? I could imagine if something fails really catastrophically the last/only frame might still be useful?
There was a problem hiding this comment.
➡️ We already capture the final screenshot as a .png (built-in feature of the harness)
src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/ScreenshotInProcess.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/IntegrationTest/New.IntegrationTests/InProcess/ScreenshotInProcess.cs
Outdated
Show resolved
Hide resolved
| Application.Current.MainWindow.LayoutUpdated -= OnThrottledInteraction; | ||
| Application.Current.MainWindow.PreviewMouseMove -= OnThrottledInteraction; | ||
| Application.Current.MainWindow.PreviewMouseDown -= OnInteraction; | ||
| Application.Current.MainWindow.PreviewKeyDown -= OnInteraction; |
There was a problem hiding this comment.
What might be useful is if the images captured for the key actions would also write to the picture which key was being pressed.
There was a problem hiding this comment.
(this can of course be a follow up!)
There was a problem hiding this comment.
➡️ Yes, there are a few potential improvements like this. I think the most important early improvement would be frame differencing/cropping to reduce the output size.
| lock (s_frames) | ||
| { | ||
| s_frames.Clear(); | ||
| } |
There was a problem hiding this comment.
Why clearing if a one-off capture fails? Maybe instead write a black frame or something so a transient failure is understood to be transient?
There was a problem hiding this comment.
➡️ InitializeCoreAsync is called at the start of a new test. It would be misleading to preserve frames from a prior test here.
| Contract.ThrowIfTrue(idat.Count == 0); | ||
| Contract.ThrowIfTrue(iend.IsEmpty); | ||
|
|
||
| return (signature, ihdr, srgb, gama, phys, idat.ToImmutableArrayOrEmpty(), iend); |
There was a problem hiding this comment.
Why are we returning a bunch of spans we just asserted are empty?
There was a problem hiding this comment.
➡️ We asserted they are not empty.
| static ScreenshotInProcess() | ||
| { | ||
| DataCollectionService.RegisterCustomLogger( | ||
| static fullPath => |
There was a problem hiding this comment.
What value would be passed as fullPath? If multiple tests fail will they have clear different names in CI?
There was a problem hiding this comment.
➡️ The full path is determined by the test harness. The test name is part of the full path, so outputs from different tests can generally be identified. There are some exceptions (e.g. theories where inputs change but the test has the same name), but improvements in this space would be owned by microsoft/vs-extension-testing.
| var firstEncoded = EncodeFrame(firstFrame); | ||
|
|
||
| // PNG Signature (8 bytes) | ||
| WritePngSignature(fileStream, buffer); |
There was a problem hiding this comment.
A unrelated question:
Why choose APNG instead of Gif?
There was a problem hiding this comment.
➡️ PNG preserves the full image quality and seemed easy
| Over = 1, | ||
| } | ||
|
|
||
| static ScreenshotInProcess() |
There was a problem hiding this comment.
If I run the test locally, will it also record the screen?
There was a problem hiding this comment.
Also, does it only collect when test fails? (I didn't see the change, did you make any thing in the extension package?)
There was a problem hiding this comment.
If I run the test locally, will it also record the screen?
Yes
Also, does it only collect when test fails?
➡️ No, but it only saves to disk when the test fails
|
@Cosifne @jasonmalinowski @davidwengier I believe all feedback responded to now |
| goto case HotReloadAction.Edit; | ||
|
|
||
| case HotReloadAction.Edit: | ||
| //if (!isManaged) |
There was a problem hiding this comment.
Do we want to keep these commented code?
There was a problem hiding this comment.
Also there are commented code in other cases.
There was a problem hiding this comment.
➡️ Yes (keep), mainly because if we ever need these, the text in the comment gives something to search for the reference implementation.
| <Name>VisualStudioSetup</Name> | ||
| <Private>False</Private> | ||
| </ProjectReference> | ||
| <ProjectReference Include="..\..\..\ExpressionEvaluator\Package\ExpressionEvaluatorPackage.csproj" Name="ExpressionEvaluatorPackage" Private="false" /> |
There was a problem hiding this comment.
Why it needs to reference ExpressionEvaluator pacakge now?
There was a problem hiding this comment.
➡️ It was always needed, but we forgot it previously.
|
Merging to improve stability and diagnosabliity of integration tests. @jasonmalinowski if you have any other questions please feel free to reach out for follow-up. |
ghost
added this to the 
This is generally a workaround for #70938.