Make ALL template warnings show up as test errors (not just C# warnings), and produce more diagnostic artifacts #17637

Eilon · 2023-09-25T22:29:23Z

Right now we correctly set TreatWarningsAsErrors=true for template tests, but that catches only C# compiler warnings and converts those to errors. It does not treat MSBuild warnings as errors. So, any warning during build/publish/whatever that isn't a C# compiler warning is ignored during our tests. And I think this is why #17561 lingered un-caught for a while.

This PR has a few parts:

Make sure MSBuild warnings fail the build with a specific warnaserror MSBuild option. This is different from the C# TreatWarningsAsErrors=true option. There's info here on that: https://stackoverflow.com/a/47448331/31668
Address current warnings that we've been missing or aren't worried about: there are two of these, which you can see in the excluded warnings in the test code.
Have the build produce more artifacts even during successful runs. I spent hours trying to get a binlog for successful template test runs (I can't run them locally for an unknown reason). We now attach artifacts for all test runs (success or failure) and you can get test console output as well as binlogs to view/download. With this PR you can download binlogs for any template test and then view locally with https://msbuildlog.com/ online or with an installable app

cc @mattleibow @PureWeen

(This PR is a replacement for #17586.)

- Ensure all template build warnings are caught as errors and make the tests fail - Save template test output as artifacts

Eilon · 2023-09-26T20:06:12Z

Also tagging @pjcollins because it looks like most of this test infrastructure was introduced in #13662.

Eilon · 2023-09-26T20:52:52Z

src/TestUtils/src/Microsoft.Maui.IntegrationTests/BaseBuildTest.cs

@@ -98,25 +98,10 @@ public void BuildTestSetUp()
 		[TearDown]
 		public void BuildTestTearDown()
 		{
-			// Clean up test or attach content from failed tests


I deleted the code that would clean up test artifacts if the test passed. As it turned out, tests were passing despite there being bugs in the product, but successful tests had no logs. With this change, every template test has binlogs and console output making it a lot easier to understand why a test passed even though perhaps it should have failed.

What happens if we re-run ? this would previous delete the previous results. My issue here is if they can get mixed up if have similar names. Do you know if we add the DateTime stamp to these logs?

Rerun I think is fine because it presumably always gets a clean instance, no? I assume that because there are all kinds of ways test can fail without cleanup running and I'd hope that wouldn't cause a rerun to fail?

Eilon · 2023-09-26T20:56:18Z

src/TestUtils/src/Microsoft.Maui.IntegrationTests/TemplateTests.cs

@@ -123,12 +123,11 @@ public void PackCoreLib(string id, string framework, string config)
 			EnableTizen(projectFile);
 			FileUtilities.ReplaceInFile(projectFile, new Dictionary<string, string>()
 			{
-				{ "UseMaui", "UseMauiCore" },


This was here from the original tests introduced in #13662. I took it out because:

It causes the template build to produce the notorious MA002 MAUI MSBuild warning (which I made into an error for the test). The warning was produced because the template is authored to have a correct Microsoft.Maui.Controls reference (to match UseMaui), but this test changes UseMaui to UseMauiCore, but without the required Microsoft.Maui reference, thus producing this warning (which becomes an error).

It's apparently unnecessary (the test passes now without any warnings)

Do we have a test for usemauicore still. This is a feature we need. Probably not essential anymore since we have nugets, but it should only add half of maui

We might not have such a test now, but I think for this template test it's best to test the template as close to reality as possible (because that's what customers use). If we want an additional test scenario I think that should be a separate test.

Also I think I mentioned somewhere that the test as-is was somewhat incorrect anyway, because it specified UseMauiCore without adding the correct package reference, which is something we literaly warn people not to do 😄

For some more context we do test the mauilib template without any modifications as part of the Build test here:

maui/src/TestUtils/src/Microsoft.Maui.IntegrationTests/TemplateTests.cs

Lines 26 to 30 in ff140e5

[TestCase("mauilib", DotNetPrevious, "Debug", true)]

[TestCase("mauilib", DotNetPrevious, "Release", true)]

[TestCase("mauilib", DotNetCurrent, "Debug", true)]

[TestCase("mauilib", DotNetCurrent, "Release", true)]

public void Build(string id, string framework, string config, bool shouldPack)

Maybe we should just put an [Ignore] attribute on the PackCoreLib test for now and fix it in a follow up PR?

Edit - I just saw the related comment below, I think it would also be fine to merge this as is and fix it separately.

@mattleibow alright I put back this test, but also made it change the MAUI Controls reference to be a MAUI Core reference.

Eilon · 2023-09-26T20:57:33Z

src/TestUtils/src/Microsoft.Maui.IntegrationTests/Utilities/DotnetInternal.cs

@@ -34,6 +34,26 @@ public static bool Build(string projectFile, string config, string target = "",
 				binlogPath = Path.Combine(Path.GetDirectoryName(projectFile) ?? "", binlogName);
 			}

+			if (msbuildWarningsAsErrors)


This section is what now causes the template tests to treat MSBuild warnings as errors. There are two excluded warnings (see below, with justification). I originally had the sample tests also do this, but our samples are not nearly as cleanly authored, and aren't as important, so the samples tests do NOT treat MSBuild warnings as errors.

Eilon · 2023-09-26T20:58:04Z

src/TestUtils/src/Microsoft.Maui.IntegrationTests/Utilities/DotnetInternal.cs

-				TestContext.WriteLine(runOutput);
+			TestContext.WriteLine($"Process exit code: {exitCode}");
+			TestContext.WriteLine($"-------- Process output start --------");
+			TestContext.WriteLine(runOutput);


This produces more details in the console log output of the tests, which greatly aid in debugging tests even when they pass.

pjcollins

Changes here make sense and LGTM assuming CI is also happy

Eilon · 2023-09-27T00:27:27Z

Changes here make sense and LGTM assuming CI is also happy

Thanks @pjcollins . Do you happen to have any idea about why this setting was changed for the test? https://github.com/dotnet/maui/pull/17637/files#r1337777334 . It ends up causing the MA002 warning because it creates a semi-invalid CSPROJ file. And the test works fine without it.

Eilon · 2023-09-27T04:28:08Z

Alright, tests all pass. @mattleibow / @PureWeen - do either of you want to take a quick look before I merge?

pjcollins · 2023-09-27T15:53:20Z

Thanks @pjcollins . Do you happen to have any idea about why this setting was changed for the test? https://github.com/dotnet/maui/pull/17637/files#r1337777334 . It ends up causing the MA002 warning because it creates a semi-invalid CSPROJ file. And the test works fine without it.

This property was added when porting over the "core library" test here: https://github.com/dotnet/maui/pull/13662/files#diff-f987df2e4d48c04c42bb417e36d0d8b1fa9ddd29f10459c98f16d7b9b10f47c5L195. As Matthew mentioned above we may want to keep it and ignore the warning for that specific test to ensure we have a test covering this scenario?

Eilon · 2023-09-29T18:57:41Z

The UseMauiCore test is restored to its original glory, but I updated it to have a CSPROJ that correctly avoids MA002 by updating the package references. So, now this PR is not changing which scenarios the tests cover.

Eilon · 2023-10-02T16:27:36Z

@rmarinho / @mattleibow - this PR requires an approval to merge. Are there any further changes required to this PR? (This test would have helped prevent several bugs that shipped in previews, so I'd love to have it in place to prevent future bugs.)

…gs), and produce more diagnostic artifacts (#17637) - Ensure all template build warnings are caught as errors and make the tests fail - Save template test output as artifacts even if they pass

Ensure template errors are found and diagnoseable

095160e

- Ensure all template build warnings are caught as errors and make the tests fail - Save template test output as artifacts

Eilon added the area/templates 📄 Project templates, Item Templates for Blazor and MAUI label Sep 25, 2023

Eilon requested review from mattleibow and PureWeen September 25, 2023 22:29

Eilon marked this pull request as ready for review September 25, 2023 22:29

Eilon requested a review from a team as a code owner September 25, 2023 22:29

Eilon mentioned this pull request Sep 25, 2023

Make ALL template warnings show up as test errors (not just C# warnings), and produce more diagnostic artifacts #17586

Closed

Ignore samples tests, try to fix template test

32acc3d

Eilon requested a review from pjcollins September 26, 2023 20:05

Eilon commented Sep 26, 2023

View reviewed changes

Update TemplateTests.cs

a215f8f

Eilon commented Sep 26, 2023

View reviewed changes

pjcollins previously approved these changes Sep 26, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

Eilon enabled auto-merge (squash) September 27, 2023 16:19

Fix MAUI Core Lib test

37e37e9

Eilon dismissed pjcollins’s stale review via 37e37e9 September 29, 2023 18:46

Update TemplateTests.cs

6477751

mattleibow approved these changes Oct 2, 2023

View reviewed changes

Eilon merged commit 1ea2808 into main Oct 2, 2023
47 checks passed

Eilon deleted the eilon/template-errors-catcher branch October 2, 2023 17:41

github-actions bot locked and limited conversation to collaborators Dec 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ALL template warnings show up as test errors (not just C# warnings), and produce more diagnostic artifacts #17637

Make ALL template warnings show up as test errors (not just C# warnings), and produce more diagnostic artifacts #17637

Eilon commented Sep 25, 2023

Eilon commented Sep 26, 2023

Eilon Sep 26, 2023

rmarinho Sep 27, 2023

Eilon Sep 27, 2023

Eilon Sep 26, 2023

mattleibow Sep 27, 2023

Eilon Sep 27, 2023

Eilon Sep 27, 2023

pjcollins Sep 27, 2023 •

edited

Eilon Sep 29, 2023

Eilon Sep 26, 2023

Eilon Sep 26, 2023

pjcollins left a comment

Eilon commented Sep 27, 2023

Eilon commented Sep 27, 2023

pjcollins commented Sep 27, 2023

This comment was marked as outdated.

Eilon commented Sep 29, 2023

Eilon commented Oct 2, 2023

	[TestCase("mauilib", DotNetPrevious, "Debug", true)]
	[TestCase("mauilib", DotNetPrevious, "Release", true)]
	[TestCase("mauilib", DotNetCurrent, "Debug", true)]
	[TestCase("mauilib", DotNetCurrent, "Release", true)]
	public void Build(string id, string framework, string config, bool shouldPack)

Make ALL template warnings show up as test errors (not just C# warnings), and produce more diagnostic artifacts #17637

Make ALL template warnings show up as test errors (not just C# warnings), and produce more diagnostic artifacts #17637

Conversation

Eilon commented Sep 25, 2023

Eilon commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcollins Sep 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcollins left a comment

Choose a reason for hiding this comment

Eilon commented Sep 27, 2023

Eilon commented Sep 27, 2023

pjcollins commented Sep 27, 2023

This comment was marked as outdated.

Eilon commented Sep 29, 2023

Eilon commented Oct 2, 2023

pjcollins Sep 27, 2023 •

edited