Skip to content

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Nov 12, 2019

Addresses #11064

PR adds gRPC interop tests.

  • Code is in src/Grpc directory
  • Code is "disconnected" from the rest of build infrastructure through blank directory props/targets. If required the test project could be made to integrate with the rest of the build by moving the blank directory props/targets files to testassets.
  • The interop client and server are completely independent projects, invoked by tests with dotnet run
  • Tests run in a minute on my machine. Could speed them up by publishing client and running exe rather than calling dotnet run 18 times. I have kept things simple for now.
  • Server runs with Kestrel. In the future we'll want to pass args to start server with HttpSys

Is this a good approach?
What changes need to happen elseware to run test project in ProjectTemplates job?

@github-actions github-actions bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 12, 2019
@JunTaoLuo JunTaoLuo added area-grpc Includes: GRPC wire-up, templates and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Nov 12, 2019
@github-actions github-actions bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 13, 2019
@JamesNK JamesNK requested a review from Pilchie November 13, 2019 00:06
@JamesNK
Copy link
Member Author

JamesNK commented Nov 13, 2019

@Pilchie Some of the files in this PR are from grpc-dotnet. They all exist under /src/Grpc/test/testassets. None of this code runs outside of testing, and it doesn't ship.

What third-party-code actions are required?

@JamesNK JamesNK removed the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 13, 2019
@JamesNK JamesNK changed the title [WIP] Add gRPC interop tests Add gRPC interop tests Nov 13, 2019
@github-actions github-actions bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 13, 2019
@JamesNK
Copy link
Member Author

JamesNK commented Nov 13, 2019

The build passes. What are the next steps here? Is it a matter of merging the PR then updating project template job tasks to add a task to run test project? Or are there changes required to the other parts of the build to handle these new projects under /src/grpc?

I don't know what changes are required from here on. I need build SMEs to guide me 🙏

@Pilchie
Copy link
Member

Pilchie commented Nov 13, 2019

Ideally, you place the external code in a top level folder that makes it clear that it is external, and a notice added to https://github.com/aspnet/AspNetCore/blob/master/THIRD-PARTY-NOTICES.txt.

}
}

#region TestData
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooo, spicy. I'll let @davidfowl get mad at you.

Copy link
Contributor

@jkotalik jkotalik left a comment

Choose a reason for hiding this comment

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

Don't see any reason to block this.

@JamesNK JamesNK requested a review from a team as a code owner November 15, 2019 19:20
@github-actions github-actions bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Nov 15, 2019
@JamesNK
Copy link
Member Author

JamesNK commented Nov 16, 2019

@JunTaoLuo Some outstanding errors. How do I resolve these?

Build error:

C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin\Roslyn\Microsoft.Managed.Core.targets(102,5): error : SourceRoot items must include at least one top-level (not nested) item when DeterministicSourcePaths is true [F:\workspace.9\_work\1\s\src\Grpc\test\testassets\InteropClient\InteropClient.csproj]
##[error]C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin\Roslyn\Microsoft.Managed.Core.targets(102,5): error : SourceRoot items must include at least one top-level (not nested) item when DeterministicSourcePaths is true

Test error (one sample, this was thrown for for every test):

System.TimeoutException : The operation at /_/src/Grpc/test/InteropTests/InteropTests.cs:43 timed out after reaching the limit of 30000ms.
   at Microsoft.AspNetCore.Testing.TaskExtensions.TimeoutAfter(Task task, TimeSpan timeout, String filePath, Int32 lineNumber)
   at InteropTests.InteropTests.InteropTestCase(String name) in /_/src/Grpc/test/InteropTests/InteropTests.cs:line 45
--- End of stack trace from previous location ---
Output:
[ERROR] It was not possible to find any compatible framework version
[ERROR] The framework 'Microsoft.NETCore.App', version '5.0.0-alpha1.19521.2' was not found.
[ERROR]   - The following frameworks were found:
[ERROR]       5.0.0-alpha.1.19551.1 at [F:\workspace.9\_work\1\s\.dotnet\shared\Microsoft.NETCore.App]
[ERROR]       5.0.0-alpha.1.19562.8 at [F:\workspace.9\_work\1\s\.dotnet\shared\Microsoft.NETCore.App]
[ERROR] 
[ERROR] You can resolve the problem by installing the specified framework and/or SDK.
[ERROR] 
[ERROR] The specified framework can be found at:
[ERROR]   - https://aka.ms/dotnet-core-applaunch?framework=Microsoft.NETCore.App&framework_version=5.0.0-alpha1.19521.2&arch=x64&rid=win10-x64

The test error happened when calling dotnet run on the interop client/server.

@JunTaoLuo
Copy link
Contributor

I think we might need to add something like this to the interop tests to pick up the latest shared framework: https://github.com/aspnet/AspNetCore/blob/master/src/ProjectTemplates/test/Infrastructure/TemplateTests.props.in.

Essentially we need the workaround here: https://github.com/aspnet/AspNetCore/blob/master/Directory.Build.targets#L115-L122 to be able to run on the latest shared framework. However, I believe the repo has been "isolated" so that fix isn't picked up and the latest shared framework cannot be found.

@JamesNK
Copy link
Member Author

JamesNK commented Nov 25, 2019

@JunTaoLuo Can you take this over - just need to get them passing in CI environment.

@JunTaoLuo JunTaoLuo force-pushed the jamesnk/interoptests branch from 50b4253 to e25ee07 Compare January 30, 2020 22:26
@JunTaoLuo
Copy link
Contributor

@HaoK The tests are passing on Helix! I'm doing one more pass to do a clean up to remove the AzDO modifications.

@HaoK
Copy link
Member

HaoK commented Mar 20, 2020 via email

@JunTaoLuo
Copy link
Contributor

@JamesNK @dotnet/aspnet-build @HaoK I'm planning on merging this once tests pass. Anything else I should address before I do that?

@JamesNK
Copy link
Member Author

JamesNK commented Mar 20, 2020

:shipit:

I would approve but it is my own PR 😄

@JunTaoLuo JunTaoLuo merged commit 8afb78f into master Mar 21, 2020
@JunTaoLuo JunTaoLuo deleted the jamesnk/interoptests branch March 21, 2020 02:32
Pilchie added a commit that referenced this pull request Mar 21, 2020
ghost pushed a commit that referenced this pull request Mar 21, 2020
JunTaoLuo pushed a commit that referenced this pull request Mar 23, 2020
JunTaoLuo pushed a commit that referenced this pull request Mar 23, 2020
* Revert "Revert "Add gRPC interop tests (#17040)" (#20047)"

This reverts commit 236dcd9.

* Fix daily and quarantine Helix runs

* Cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-grpc Includes: GRPC wire-up, templates area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants