[Templating] Fix templating infrastructure and add additional tests for new scenarios #8275

javiercn · 2019-03-07T12:34:59Z

Improved selenium start and tear down
- Selenium is set up and torn down in an assembly fixture.
- Selenium is initialized lazily and in a non-blocking way.
- Selenium processes are tracked as part of the build and their pids
  written to a file on disk for cleanup in the event of unexpected
  termination of the test process.
A sentinel process is started along Selenium and will kill the process if it didn't finish correctly after 1 hour and 30 minutes.
Browser fixture retries with linear backoff to create a remote
driver. Under heavy load (like when we are doing a simultaneous NPM
restore) the selenium server can become unresponsive so we retry
three times, with a longer comand timeout allowance each time up to
a max of 3 minutes.
Moved test project setup to build time instead of runtime.
- Added target PrepareForTest to create the required files for testing
  - The template creation folder.
  - The template props file to use our built packages.
  - The folder for the custom hive.
- Added assembly metadata attributes to find all the data we need to
  run the tests.
  - Path to the artifacts shipping packages folder.
  - Path to the artifacts non-shipping packages folder.
  - Path to the test templates creation folder.
  - Path to use for the custom templating hive used in tests.
- Proper cleanup as part of the build
  - Remove the test templates creation folder.
  - Remove the test packages restore path.
  - Recreate the test templates creation folder.
  - Recreate the test packages restore path.
- Generated Directory.Build.Props and Directory.Build.Targets in the
  test templates creation folder.
- Cleaned up potentially stale templatetestsprops.
Improved test flows
- Initialization is done lazily and asynchronously.
  - Selenium
  - Browser fixture
  - Template initialization.
- Flattened test flows to avoid assertions inside deep callstacks.
  - All assertions happen at the test level with improved error messages.
    - With the exception of the migrations assertions.
  - Assertions contain information about which step failed, for what
    project and what failure details.
- Broke down tests to perform individual steps instead of mixing build
  and publish.
  - Publish project.
  - Build project. (Debug)
  - Run built project.
  - Run published project.
- Concentrated build logic into the Project class.
  - Context between the different steps of a test is maintained in
    this class.
  - All operations that require coordination are performed within this
    class.
    - There is a lock for dotnet and a lock for nodejs. When building
      SPAs we acquire the nodejs lock to correctly prevent multiple
      runs of nodejs in parallel.

[ApiAuthorization template cleanups]

Fix preview3 issues with breaking changes on Entity framework by
manually configuring the model in ApiAuthorizationDbContext.
Add app.db to the project file when using local db.
Fix linting errors on angular template.
Fix react tests
Add tests to cover new auth options in the SPA templates.

This is an example of what a failing test looks like now.

Project new react  --auth Individual failed to publish.
Process exited with code 1
StdErr: 
StdOut: Microsoft (R) Build Engine version 16.1.4-preview+ga972ec96c3 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restoring packages for C:\work\AspNetCore\src\ProjectTemplates\test\bin\Debug\netcoreapp3.0\TestTemplates\AspNet.reactindividual.c7129e\AspNet.reactindividual.c7129e.csproj...
  Installing Microsoft.NET.Sdk.Razor 3.0.0-preview4.19162.2.
C:\work\AspNetCore\.dotnet\x64\sdk\3.0.100-preview4-010309\NuGet.targets(114,5): error : Access to the path 'Microsoft.AspNetCore.Razor.Language.dll' is denied. [C:\work\AspNetCore\src\ProjectTemplates\test\bin\Debug\netcoreapp3.0\TestTemplates\AspNet.reactindividual.c7129e\AspNet.reactindividual.c7129e.csproj]

See that all the details are on the first line, what part of the test exactly failed and what template + arguments failed.

Project new react --auth Individual failed to publish.

ryanbrandenburg

My only serious concern is the FetchData thing, the rest are just opinions and suggestions.

ryanbrandenburg · 2019-03-15T18:06:43Z

src/ProjectTemplates/test/BaselineTest.cs

+            Output = output;
        }

        public Project Project { get; set; }


You may as well get rid of this it you're not generating the project in the constructor.

ryanbrandenburg · 2019-03-15T18:17:52Z

src/ProjectTemplates/test/MvcTemplateTest.cs

        }

-        public Project Project { get; }
+        public Project Project { get; set; }


Again, no point in this if its only use is in the test that created it.

This will be used when I break the test into multiple stages.

src/ProjectTemplates/test/SpaTemplateTest/AngularTemplateTest.cs

src/ProjectTemplates/test/SpaTemplateTest/ReactTemplateTest.cs

src/ProjectTemplates/test/SpaTemplateTest/SpaTemplateTestBase.cs

src/Shared/E2ETesting/BrowserFixture.cs

src/Shared/E2ETesting/WaitAssert.cs

src/ProjectTemplates/test/AssemblyInfo.AssemblyFixtures.cs

dougbu

Like the consistent updates to the test classes but didn't review that part or the fixture in depth. Defer to @SteveSandersonMS there.

ryanbrandenburg · 2019-03-18T19:23:55Z

src/ProjectTemplates/test/EmptyWebTemplateTest.cs


            using (var aspNetProcess = Project.StartBuiltProjectAsync())
            {
+                Assert.False(aspNetProcess.Process.HasExited, ErrorMessages.GetErrorMessage("Run built project", Project, aspNetProcess.Process);


Considering all the places we're copying this let's just put it inside AssertOk.

Nope, the asserts are better of at the test method level so that:

You see a very specific error message about what test failed and why.

You get the exact line of code where the test blew up instead of having to dig through the call stack.

DRY is good for product code, not for tests.

Hitting these endpoints is unlikely to have anything to do with why the process has stopped, and if that's what we're testing for oughtn't these checks run after we actually do something?

This is checking that the process didn't exit early. Like when you do dotnet run and there is a framework mismatch or similar.

src/Components/test/E2ETest/ServerExecutionTests/ServerSideAppTest.cs

src/Shared/E2ETesting/BrowserTestBase.cs

rynowak · 2019-03-18T20:38:29Z

src/Components/test/E2ETest/ServerExecutionTests/ServerSideAppTest.cs


            // Verify we're now on the counter page, with that nav link (only) highlighted
-            WaitAssert.Equal("Counter", () => Browser.FindElement(mainHeaderSelector).Text);
+            Browser.Equal("Counter", () => Browser.FindElement(mainHeaderSelector).Text);


What's the reason for changing these?

They were relying on an async local and I wanted to avoid that as much as possible. I considered either passing the browser explicitly or making them extension methods, and I did the second because it felt more "idiomatic".

I didn't think a lot of one way or the other. Just wanted the async local usage limited to the minimum.

But isn't this a big change in functionality? If these needed waitassert in the past, why is this safe to remove?

I didn't remove anything. I transformed the method.
This used to be WaitAssert.Equal(string expected, Func func) and internally used BrowserTestBase.Browser (an async local).

I converted it into WaitAssert.Equal(IWebDriver browser, string expected, Func func) and to make it more similar to what it was I made WaitAssert.Equal and related methods extension methods on IWebDriver, so WaitAssert.Equal(this IWebDriver browser, string expected, Func<string> func).

Functionality wise they are equivalent. The use of an async local was unnecessary, when we can just pass the browser parameter explicitly.

rynowak

for the components changes

SteveSandersonMS · 2019-03-19T15:02:51Z

...rojectTemplates/Web.Spa.ProjectTemplates/content/React-CSharp/.template.config/template.json

This exclude-based approach looks kind of risky, as in the future we might add more files and not realise they also have to be included in the exclude list. Two possibilities:

Do we already have tests that assert the template output doesn't include any extra unexpected files? If so we're probably fine already.

If not, maybe we should establish a naming convention like internal.setupTests.js and then have an exclude pattern like **\internal.* so someone who's just copying/pasting existing patterns would automatically have their new files exclued.

Not a massively big deal - just wanted to get your thoughts on this.

Oh it looks like we do have baseline tests.

SteveSandersonMS · 2019-03-19T15:03:29Z

src/ProjectTemplates/Web.Spa.ProjectTemplates/content/React-CSharp/ClientApp/src/App.test.js

Should this new line be here? It doesn't seem necessary for the test.

It is, when we are doing auth the test becomes async and fails if not. (Throws indicating that the component has been unmounted). Technically a promise resolve method passed to ReactDOM.Render as third parameter should work, but empirically it doesn't. The timeout makes it reliable.
I could have queued this only for the auth case, but I think this way is a much better starting point, as this test will fail the moment you add a single http request to your app and it's not easy to figure out.

SteveSandersonMS · 2019-03-19T15:04:19Z

...tTemplates/content/React-CSharp/ClientApp/src/components/api-authorization/AuthorizeRoute.js

Nit: Missing semicolons at the ends of all these lines.

I'm wondering why the linter doesn't complain

SteveSandersonMS · 2019-03-19T15:04:38Z

...pa.ProjectTemplates/content/React-CSharp/ClientApp/src/components/api-authorization/Login.js

SteveSandersonMS · 2019-03-19T15:04:49Z

...a.ProjectTemplates/content/React-CSharp/ClientApp/src/components/api-authorization/Logout.js

SteveSandersonMS · 2019-03-19T15:12:00Z

src/ProjectTemplates/test/Infrastructure/Directory.Build.targets.in

This seems strange. We're importing a .props file into a .targets project. Should this line be in Directory.Build.props.in instead?

I know it might work either way but it could be confusing for props-like things to be imported at an unexpected time in the build process.

It needs to be here so that the settings on the project don't override them. This is meant to override the project settings with all the stuff from our build.

SteveSandersonMS

Wow, this is super sophisticated. Glad to see you've modernised a lot of it and cleared away many of the older obsolete workarounds!

Looking forwards to a world of more reliable E2E tests.

* Improved selenium start and tear down * Selenium is set up and torn down in an assembly fixture. * Selenium is initialized lazily and in a non-blocking way. * Selenium processes are tracked as part of the build and their pids written to a file on disk for cleanup in the event of unexpected termination of the test process. * Browser fixture retries with linear backoff to create a remote driver. Under heavy load (like when we are doing a simultaneous NPM restore) the selenium server can become unresponsive so we retry three times, with a longer comand timeout allowance each time up to a max of 3 minutes. * Moved test project setup to build time instead of runtime. * Added target PrepareForTest to create the required files for testing * The template creation folder. * The template props file to use our built packages. * The folder for the custom hive. * Added assembly metadata attributes to find all the data we need to run the tests. * Path to the artifacts shipping packages folder. * Path to the artifacts non-shipping packages folder. * Path to the test templates creation folder. * Path to use for the custom templating hive used in tests. * Proper cleanup as part of the build * Remove the test templates creation folder. * Remove the test packages restore path. * Recreate the test templates creation folder. * Recreate the test packages restore path. * Generated Directory.Build.Props and Directory.Build.Targets in the test templates creation folder. * Cleaned up potentially stale templatetestsprops. * Improved test flows * Initialization is done lazily and asynchronously. * Selenium * Browser fixture * Template initialization. * Flattened test flows to avoid assertions inside deep callstacks. * All assertions happen at the test level with improved error messages. * With the exception of the migrations assertions. * Assertions contain information about which step failed, for what project and what failure details. * Broke down tests to perform individual steps instead of mixing build and publish. * Publish project. * Build project. (Debug) * Run built project. * Run published project. * Concentrated build logic into the Project class. * Context between the different steps of a test is maintained in this class. * All operations that require coordination are performed within this class. * There is a lock for dotnet and a lock for nodejs. When building SPAs we acquire the nodejs lock to correctly prevent multiple runs of nodejs in parallel. [ApiAuthorization template cleanups] * Fix preview3 issues with breaking changes on Entity framework by manually configuring the model in ApiAuthorizationDbContext. * Add app.db to the project file when using local db. * Fix linting errors on angular template. * Fix react tests * Add tests to cover new auth options in the SPA templates.

Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 7, 2019

javiercn force-pushed the javiercn/spa-test-improvements branch 5 times, most recently from 5aeb375 to 3e4fe46 Compare March 14, 2019 13:57

ryanbrandenburg reviewed Mar 15, 2019

View reviewed changes

javiercn force-pushed the javiercn/spa-test-improvements branch 5 times, most recently from 47fb4c4 to 5f663d8 Compare March 18, 2019 08:57

javiercn commented Mar 18, 2019

View reviewed changes

src/ProjectTemplates/test/AssemblyInfo.AssemblyFixtures.cs Show resolved Hide resolved

javiercn force-pushed the javiercn/spa-test-improvements branch from 6e38440 to ae0f6be Compare March 18, 2019 18:16

javiercn changed the title ~~[Templating] tmp~~ [Templating] Fix templating infrastructure and add additional tests for new scenarios Mar 18, 2019

javiercn marked this pull request as ready for review March 18, 2019 18:21

javiercn requested review from SteveSandersonMS and dougbu as code owners March 18, 2019 18:21

dougbu approved these changes Mar 18, 2019

View reviewed changes

ryanbrandenburg reviewed Mar 18, 2019

View reviewed changes

rynowak reviewed Mar 18, 2019

View reviewed changes

src/Components/test/E2ETest/ServerExecutionTests/ServerSideAppTest.cs Outdated Show resolved Hide resolved

rynowak reviewed Mar 18, 2019

View reviewed changes

src/Shared/E2ETesting/BrowserTestBase.cs Show resolved Hide resolved

rynowak reviewed Mar 18, 2019

View reviewed changes

javiercn requested a review from Tratcher as a code owner March 19, 2019 07:55

javiercn force-pushed the javiercn/spa-test-improvements branch from ae38d93 to cab3c28 Compare March 19, 2019 08:00

rynowak approved these changes Mar 19, 2019

View reviewed changes

SteveSandersonMS reviewed Mar 19, 2019

View reviewed changes

...pa.ProjectTemplates/content/React-CSharp/ClientApp/src/components/api-authorization/Login.js Outdated

Copy link

Member

SteveSandersonMS Mar 19, 2019

Choose a reason for hiding this comment

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

Semicolon

SteveSandersonMS reviewed Mar 19, 2019

View reviewed changes

...a.ProjectTemplates/content/React-CSharp/ClientApp/src/components/api-authorization/Logout.js Outdated

Copy link

Member

SteveSandersonMS Mar 19, 2019

Choose a reason for hiding this comment

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

Semicolon

SteveSandersonMS reviewed Mar 19, 2019

View reviewed changes

SteveSandersonMS approved these changes Mar 19, 2019

View reviewed changes

javiercn force-pushed the javiercn/spa-test-improvements branch from 020c640 to e854ee9 Compare March 19, 2019 19:01

javiercn merged commit 9f1a978 into master Mar 20, 2019

mkArtakMSFT mentioned this pull request Apr 4, 2019

Run templating tests on Windows #8221

Closed

natemcmaster deleted the javiercn/spa-test-improvements branch May 3, 2019 06:23

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

[Templating] Fix templating infrastructure and add additional tests for new scenarios #8275

[Templating] Fix templating infrastructure and add additional tests for new scenarios #8275

Uh oh!

Conversation

javiercn commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This is an example of what a failing test looks like now.

Uh oh!

ryanbrandenburg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rynowak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

javiercn commented Mar 7, 2019 •

edited

Loading