Skip to content

Add missing BeforeTargets for IIS local testing #40379

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

Merged
merged 4 commits into from
Feb 27, 2022

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Feb 23, 2022

No description provided.

@dougbu dougbu requested a review from HaoK February 23, 2022 17:31
@ghost ghost added the area-runtime label Feb 23, 2022
@dougbu
Copy link
Contributor Author

dougbu commented Feb 23, 2022

Thanks to @HaoK for reporting this problem in #39810

Not sure how I missed this when doing #39998. Suspect I had published before testing locally.

Copy link
Member

@HaoK HaoK left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

@dougbu
Copy link
Contributor Author

dougbu commented Feb 23, 2022

aspnetcore-quarantined-pr build #20220223.10 failed due to a transient-seeming 503 from https://registry.yarnpkg.com/ua-parser-js/-/ua-parser-js-0.7.30.tgz. Since other jobs got what they wanted from the registry and this PR shouldn't impact Helix testing at all (that already worked), I'll ignore the failure.

@HaoK
Copy link
Member

HaoK commented Feb 23, 2022

This might fix some things, but unfortunately this doesn't do the trick for the main issue:

System.AggregateException : One or more errors occurred. (Could not find a part of the path 'C:\Github\aspnetcore\src\Servers\IIS\IIS\test\testassets\InProcessWebSite\bin\Release\net7.0\publish\InProcessWebSite-Portable'.) (Operation did not succeed after 15 retries (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element))
---- System.IO.DirectoryNotFoundException : Could not find a part of the path 'C:\Github\aspnetcore\src\Servers\IIS\IIS\test\testassets\InProcessWebSite\bin\Release\net7.0\publish\InProcessWebSite-Portable'.
---- System.AggregateException : Operation did not succeed after 15 retries (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element) (Sequence contains more than one element)
-------- System.InvalidOperationException : Sequence contains more than one element

The tests still fail because no publish\InProcessWebSite-Portable type directories are created, I tried these changes out locally, both VS builds and full build.cmd still don't produce those directories which the tests expect. That is the main issue with the current test failures locally

@dougbu
Copy link
Contributor Author

dougbu commented Feb 23, 2022

Thanks for the added information @HaoK. The IIS Express tests I tried didn't require the portable profile but failed w/o this. So, this change was necessary but not sufficient. Touching this PR up…

@HaoK
Copy link
Member

HaoK commented Feb 23, 2022

If the tests don't work for you locally, you can turn off Helix via the BuildHelixPayload setting to trigger them running the old way to flush out any kinks in this PR

@dougbu
Copy link
Contributor Author

dougbu commented Feb 23, 2022

What I tested before was

  1. git clean ...
  2. .\eng\build.cmd (this doesn't publish the test assets)
  3. dotnet test /bl .\src\Servers\IIS\IIS\test\IISExpress.FunctionalTests\IISExpress.FunctionalTests.csproj (this does)

That worked fine and src\Servers\IIS\IIS\test\testassets\InProcessWebSite\bin\Debug\net7.0\publish\ contained both InProcessWebSite-Portable and InProcessWebSite-Standalone-x64 when tests completed. My second commit here should be redundant but are you doing similar things❔

@dougbu
Copy link
Contributor Author

dougbu commented Feb 23, 2022

you can turn off Helix via the BuildHelixPayload setting to trigger them running the old way to flush out any kinks in this PR

Opened #40381 as a draft to test this way in parallel. Would still like to know why test assets are being published for me but not you @HaoK

@HaoK
Copy link
Member

HaoK commented Feb 23, 2022

I usually just do src/servers/IIS/build.cmd -c release

And then I've just been doing VS - test explorer run. I haven't tried running the full root eng/build today.

There's also some issues with the fastuptodate checks that were removed in the IIS projects, I'll be adding those back, because the current behavior is that just editing the shared test cs files does not result in a rebuild, which is super confusing, as VS will happily run the old tests since it doesn't notice the shared cs files getting edited.

@dougbu
Copy link
Contributor Author

dougbu commented Feb 24, 2022

There's also some issues with the fastuptodate checks that were removed in the IIS projects, I'll be adding those back, …

/cc @Pilchie

And then I've just been doing VS - test explorer run

Thought you said somewhere that you tried testing both from the command line and in VS. How do you test from the command line❔ I'll also see if I can repro your continued issues in VS w/ and w/o my second commit here.

@dougbu
Copy link
Contributor Author

dougbu commented Feb 24, 2022

/fyi my current suspicion is that VS test explorer doesn't use the targets I added to BeforeTargets in the first commit and my second commit didn't help but I'll see… (VS was out-of-date-ish and I'm waiting.)

@Pilchie
Copy link
Member

Pilchie commented Feb 24, 2022

@HaoK can you file an issue with detailed steps of what wasn't working when you add them back? I'd like to investigate and try fixing for real again.

@HaoK
Copy link
Member

HaoK commented Feb 24, 2022

Sure filed #40387 with the steps to reproduce in main today, the repro is actually just on the managed test code, I believe there's similar issues with the native code, but its much easier to hit with just the top level SkipNonHelix on one of the common startup tests.

@dougbu dougbu force-pushed the dougbu/test.iis.locally branch from ead351d to 25bf983 Compare February 24, 2022 18:29
@dougbu
Copy link
Contributor Author

dougbu commented Feb 24, 2022

Shoot, didn't mean to update this branch. Will reset…

@dougbu dougbu force-pushed the dougbu/test.iis.locally branch 2 times, most recently from 469bd1a to 635b2a9 Compare February 25, 2022 18:45
@dougbu dougbu requested review from a team and wtgodbe as code owners February 25, 2022 18:45
Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

LGTM if it works

@dougbu
Copy link
Contributor Author

dougbu commented Feb 25, 2022

@HaoK I was eventually able to reproduce the problems you're having with testing in VS. My thanks to @MarcoRossignoli for his help understanding the root cause.

Basically, VS uses msbuild for builds but doesn't when testing. It (vstest.console.exe) instead uses the test assembly directly to discover and execute tests. My previous second commit partially handled that case but only when using Standalone-x64 profiles. This version covers Portable too.

@HaoK
Copy link
Member

HaoK commented Feb 25, 2022

Yay, I'll make these changes in my branch to see if that fixes everything too

@HaoK
Copy link
Member

HaoK commented Feb 25, 2022

This PR fixes things for me locally too!

- VS (vstest.console.exe) examines the test assembly rather than using `msbuild`
- do this for FunctionalTestWithAssets.targets as well
- otherwise, attempted to build test assets that weren't restored
- targets ran even if regular `@(ProjectReference)` items had been ignored
@dougbu dougbu force-pushed the dougbu/test.iis.locally branch from b272d4b to 9a018b9 Compare February 25, 2022 23:30
@dougbu
Copy link
Contributor Author

dougbu commented Feb 27, 2022

🎉 Success here (testing IIS mostly on Helix), success locally (including in VS), and success in #40381 (testing IIS mostly on build agents). 🎉

@dougbu dougbu merged commit 9a431ee into dotnet:main Feb 27, 2022
@dougbu dougbu deleted the dougbu/test.iis.locally branch February 27, 2022 00:24
@ghost ghost added this to the 7.0-preview3 milestone Feb 27, 2022
@dougbu dougbu mentioned this pull request Apr 6, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants