Skip to content
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

Created SmokeTest projects. #3450

Merged
merged 19 commits into from
Nov 10, 2020
Merged

Created SmokeTest projects. #3450

merged 19 commits into from
Nov 10, 2020

Conversation

azchohfi
Copy link
Contributor

@azchohfi azchohfi commented Aug 25, 2020

Fixes #3448

This new SmokeTest project builds once for each value of the ToolkitPackages variable in SmokeTests.proj.
It automatically adds the local built nuget packages and adds the MainPage.[ToolkitPackage].xaml.cs and MainPage.[ToolkitPackage].xaml file, for each project built.

With this, we can analyze the size of the appx bundle that is built for each smoke test and compare that with other built versions.

To test this:

  • Run the build.bat script to make sure you have updated nuget files locally
  • Open a VS cmd or pwsh and from the SmokeTests folder, run "msbuild .\SmokeTests.proj".
    • It will take a while to build it all, since it is set to build for Release(.Net Native) and it will generate the appxbundle files on the SmokeTests/AppPackages folder, one for each project.

PR Type

What kind of change does this PR introduce?

  • Feature
  • Build or CI related changes

What is the current behavior?

No way to analyze the assembly size of a simple project that reference each nuget package.

What is the new behavior?

The build pipeline automatically builds a simple project for each 'ToolkitPackages', allowing us to analyze the size impact of each PR, or from time to time.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@ghost
Copy link

ghost commented Aug 25, 2020

Thanks azchohfi for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker and Kyaa-dost August 25, 2020 20:48
@ghost ghost added the feature request 📬 A request for new changes to improve functionality label Aug 25, 2020
@azchohfi azchohfi force-pushed the smokeTests branch 2 times, most recently from 06f1fd2 to dd9725a Compare August 25, 2020 21:08
@azchohfi azchohfi marked this pull request as ready for review August 26, 2020 21:52
@azchohfi
Copy link
Contributor Author

image

Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Hey @azchohfi - this looks pretty interesting and I'd be curious to learn more about how this works, would you might to elaborate more on what this is doing exactly? I mean, I understand what the PR does, but I'm mostly confused about the fact that we're building with .NET Native here. The compiler there does a pretty aggressive tree shaking (dependency reduction) step, so if we're only using a very small number of APIs in each test projects don't we risk having the binary sizes not being accurate? 🤔

Is that not a concern, or is there something to avoid that that I'm not seeing? Or is this just to provide a baseline size indication for just referencing the packages, without using them in your app? And if so, then why do we need to actually have a sample usage of some APIs at all instead of just a blank MainPage file?

Sorry for the lots of questions, just honestly curious to understand this PR more 😊
Thanks!

SmokeTests/SmokeTests.proj Outdated Show resolved Hide resolved
Added a small error check on the smoke test project.
@azchohfi
Copy link
Contributor Author

These are simple smoke tests. One extra benefit of having them is known a "minimum" app size with each package.
We know that the app size is not realistic, but its close enough. We are trying to identify big increases that other PRs might bring in the future, and this will help with this investigation.

A future step will be to install these APPXs, and execute a simple test to make sure that each package actually works, and that's why we are actually calling at least one simple api from each package.

@Sergio0694
Copy link
Member

Ah I see, makes perfect sense, thanks! 😊

@michael-hawker michael-hawker added reviewers wanted ✋ for-review 📖 To evaluate and validate the Issues or PR labels Sep 1, 2020
Copy link
Member

@Sergio0694 Sergio0694 left a comment

Choose a reason for hiding this comment

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

Went through the code and it looks good, I just can't approve as I don't seem to be able to build the actual project for some reason. I'm getting an error when running the build.bat script, specifically this:

"C:\Users\Sergio\Documents\GitHub\MS_WindowsCommunityToolkit\Windows Community Toolkit.sln" (destinazione: Build) (1) -> C:\Users\Sergio\Documents\GitHub\WindowsCommunityToolkit\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj" (2) -> (destination: _GenerateProjectPriFileCore) -> GENERATEPROJECTPRIFILE : error : PRI175: 0x80070002 - Processing Resources failed with error: Can't find the specified file [C:\Users\Sergio\Documents\GitHub\WindowsCommunityToolkit\UnitTests\UnitTests.UWP\Un itTests.UWP.csproj] GENERATEPROJECTPRIFILE : error : PRI252: 0xdef00071 - File C:\Users\Sergio\Documents\GitHub\WindowsCommunityToolkit\UnitTests\UnitTests.UWP\obj\x86\Release\Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices\it\Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.Resources.Resource.resw not found. [C:\Users\Sergio\Documents\GitHub\WindowsCommunityToolkit\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]

It's probably just an issue on my machine, as the CI is runnig fine.
Other than this, the PR does look good to me! 🚀

@azchohfi
Copy link
Contributor Author

azchohfi commented Sep 2, 2020

Very weird, I've never had this particular error. Can you try doing a git clean -fdx and running it again?

@Sergio0694
Copy link
Member

I've already tried that twice, no luck 😟

I'm thinking it might be the .NET 5 Preview SDK I have installed, as that's been causing other weird issues too for me.

@azchohfi
Copy link
Contributor Author

azchohfi commented Sep 2, 2020

Weird, but it might be. Its passing on the build server ¯_(ツ)_/¯

@michael-hawker
Copy link
Member

@azchohfi got these errors in the CI:

image

@azchohfi
Copy link
Contributor Author

I'll have to bump the timeout.

@michael-hawker
Copy link
Member

@RosarioPulella great point, that's what the wiki is going to be for. So, we should open an issue there to track adding info about smoke tests, integration tests, unit tests, etc... FYI @azchohfi

@michael-hawker michael-hawker added the next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. label Oct 29, 2020
@michael-hawker
Copy link
Member

@azchohfi looks like the smoke test passed 🎉🎉🎉

But then the publishing of the build artifacts failed:

##[error]Publishing build artifacts failed with an error: Not found PathtoPublish: D:\a\1\a\SmokeTestBundles

https://dev.azure.com/dotnet/WindowsCommunityToolkit/_build/results?buildId=41467&view=logs&j=8aca735d-65b0-5361-04c8-1881e8dfcb22&t=8c8b090b-9ba5-55e0-7299-20b5b33f2e6c&l=39

☹ Thoughts?

@michael-hawker
Copy link
Member

I also thought we had discussed before, but could we split the smoke test part into a 2nd job/stage? That'd make the build report and publish to the PR feed as a separate check from the smoke test itself which could run after, eh?

image

Basically showing up as a third line, so in this last case, we'd have seen the build and the release to our feed succeed, but then the smoke test line (not here) fail?

@azchohfi
Copy link
Contributor Author

Very small issue. We've updated the min version on master, and this changed the extensions from appx* to msix*, so the SmokeTest copy task was not copying anything, making the artifact upload task fail. This should be fixed now.
Yes, we could split this, and actually it is one of the easiest one to do this since we are not using anything that we are not already uploading (nupkg).

@michael-hawker
Copy link
Member

LGTM, opened #3560 for tracking splitting this out as it's own line-item so we get build/release status faster.

michael-hawker
michael-hawker previously approved these changes Nov 10, 2020
@ghost
Copy link

ghost commented Nov 10, 2020

Hello @michael-hawker!

Because this pull request has the auto merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@michael-hawker michael-hawker dismissed their stale review November 10, 2020 17:42

Forgot wanted to check final output of the markdown package

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

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

Tested the package output in a brand-new app from the PR feed. MarkdownTextBlock working like a charm now! 🎉🎉🎉

@michael-hawker michael-hawker merged commit 1ef0ab2 into master Nov 10, 2020
@delete-merged-branch delete-merged-branch bot deleted the smokeTests branch November 10, 2020 19:36
@michael-hawker michael-hawker added this to the 7.0 milestone Nov 10, 2020
@michael-hawker michael-hawker added this to In progress in Technical 7.0 via automation Nov 10, 2020
@Nirmal4G Nirmal4G mentioned this pull request Dec 1, 2020
3 tasks
@michael-hawker michael-hawker moved this from In progress to Done in Technical 7.0 Feb 25, 2021
@ghost ghost added the Completed 🔥 label Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merge ⚡ CI/pipeline 🔬 Completed 🔥 feature request 📬 A request for new changes to improve functionality for-review 📖 To evaluate and validate the Issues or PR next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. testing 🏗️
Projects
No open projects
Technical 7.0
  
Done
Development

Successfully merging this pull request may close these issues.

[Testing] Add Analysis for Assembly Sizes
4 participants