Skip to content

Add tests for memory leak#781

Closed
j0shuams wants to merge 7 commits intomasterfrom
jlarkin/leak-repros
Closed

Add tests for memory leak#781
j0shuams wants to merge 7 commits intomasterfrom
jlarkin/leak-repros

Conversation

@j0shuams
Copy link
Contributor

@j0shuams j0shuams commented Mar 31, 2021

This PR consolidates the leak repro samples/tests from @manodasanW 's topic branch into the existing WinUI tests/samples we have. This cuts down on the size of the solution primarily, and also acts as the first step in merging leak fixes.

@j0shuams j0shuams requested a review from manodasanW March 31, 2021 20:16
@manodasanW manodasanW mentioned this pull request Apr 1, 2021
@j0shuams j0shuams force-pushed the jlarkin/leak-repros branch from 24019d4 to 82f2a98 Compare April 1, 2021 06:12
@Scottj1s
Copy link
Member

Scottj1s commented Apr 1, 2021

let's not use the sample for leak tests, as the pipeline won't automate it. @azchohfi recently implemented mstest support for WinUI. It should land on nuget.org any day:
https://www.nuget.org/packages/MSTest.TestFramework/

Ideally, we can take the ObjectLifetimeTests, add a few more, and run them as proper unit tests.

@manodasanW
Copy link
Member

let's not use the sample for leak tests, as the pipeline won't automate it. @azchohfi recently implemented mstest support for WinUI. It should land on nuget.org any day:
https://www.nuget.org/packages/MSTest.TestFramework/

Ideally, we can take the ObjectLifetimeTests, add a few more, and run them as proper unit tests.

I agree with that, but at the same time I think until we have those, we need a set of scenarios that we can quickly run through to demonstrate as we do leak fixes that we haven't regressed anything and I feel like these samples help with that. What do you think?

@Scottj1s
Copy link
Member

Scottj1s commented Apr 1, 2021

let's not use the sample for leak tests, as the pipeline won't automate it. @azchohfi recently implemented mstest support for WinUI. It should land on nuget.org any day:
https://www.nuget.org/packages/MSTest.TestFramework/
Ideally, we can take the ObjectLifetimeTests, add a few more, and run them as proper unit tests.

I agree with that, but at the same time I think until we have those, we need a set of scenarios that we can quickly run through to demonstrate as we do leak fixes that we haven't regressed anything and I feel like these samples help with that. What do you think?

I'm okay with an IOU - let's drop a comment in the samples to that effect, and open an issue to back this all out when we add unit tests.

@j0shuams j0shuams force-pushed the jlarkin/leak-repros branch from ee8f4c7 to 1f15e44 Compare April 1, 2021 20:23
@j0shuams
Copy link
Contributor Author

j0shuams commented Apr 1, 2021

Opened #784 per discussion here


<StackPanel>
<Button Content="Navigate to SecondPage" Click="Button_Click" />
<TextBlock x:Name="Status">Statu</TextBlock>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my typo, should be Status.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will get this in final commit too

@azchohfi
Copy link
Contributor

azchohfi commented Apr 2, 2021

Copy link
Member

@Scottj1s Scottj1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given Alex's note about winui test support on nuget.org now, can we avoid this churn and just move up directly?

Copy link
Member

@Scottj1s Scottj1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider winuitest support in latest MSTest.TestFramework

@manodasanW manodasanW closed this Jan 13, 2022
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.

4 participants