dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.

https://asp.net

MIT License

35.37k stars 9.99k forks source link

Re-enable templating tests in 3.0 #13497

Closed JunTaoLuo closed 5 years ago

JunTaoLuo commented 5 years ago

I turned off the template tests to unblock https://github.com/aspnet/AspNetCore/pull/12977. We need to make sure these tests are turned back on ASAP.

Pilchie commented 5 years ago

Adding @mkArtakMSFT here. Is this something someone from your team could look at?

mkArtakMSFT commented 5 years ago

@javiercn I know you've done a lot of changes in this area during our infrastructure effort, which is still going on. I'm wondering whether this is already fixed. If not, can you please re-enable these?

javiercn commented 5 years ago

I added a change to capture binlogs on the CI. It seems that you folks broke this as part of the infrastructure update.

javiercn commented 5 years ago

@mkArtakMSFT Please reassign, as this is build infrastructure specific.

mkArtakMSFT commented 5 years ago

@JunTaoLuo and I talked and he will follow up with @javiercn to understand what exactly is broken regarding change to capture binlogs on the CI. As for this issue, we just need to re-enable the disabled tests, @javiercn. I suggest we track these two changes separately, so feel free to file a new issue, if necessary, for the investigation.

JunTaoLuo commented 5 years ago

This has to do with the SDK that's being used. To build our 5.0 projects, we have to apply some workarounds until the SDK has ingested a version of our AspNetCore shared framework. That was done in https://github.com/dotnet/core-sdk/pull/4391 so we should be able update the SDK and resolve this issue. If a new SDK is not available, I can point you to the workaround we have in place which needs to be copied locally to the temporary test project. But as @mkArtakMSFT mentioned, this should not be related to the original issue.

javiercn commented 5 years ago

How long will it take for the new SDK to be available? At that point that would solve the issue, won’t it? How involved is the workaround?

javiercn commented 5 years ago

@JunTaoLuo Just to clarify things.

What's broken is that the E2E template tests got disabled in the PR above.
I made a change to enable capturing binlogs and allow troubleshooting what was going on. (Its merged in 3.1 and I cherry-picked into a branch from master in a separate PR that tried to re-enable the tests to troubleshoot it)
As a result of that I took the screenshot above from one of the failing binlogs.

There is no issue with the binlogs. Everything works just fine, but the template tests and still broken and disabled in master, which is what we are trying to solve. The questions that I have are:

How do we apply the workaround you mention in https://github.com/dotnet/core-sdk/pull/4391?
Will this issue go away once we ingest a new SDK?
- If that's the case, how long will it take?

I'm happy if the outcome from this is we can just wait a few days after we ingest a new SDK and then re-enable the tests. Could we tie the two things together? (Ingest the SDK and reenable the E2E tests on the same PR).

Otherwise, can you help in applying the workaround?

JunTaoLuo commented 5 years ago

I just checked the latest SDK and it has already ingested a 5.0 AspNetCore shared framework so updating the SDK to 5.0.100-alpha1-014713 should fix the issue you're seeing in the template tests. And yes, you should update the SDK when re-enabling the template tests.

javiercn commented 5 years ago

@JunTaoLuo doesn’t the build update the SDK?

JunTaoLuo commented 5 years ago

Theoretically yes but in practice, no. We get automatic updates from Arcade, which specifies a minimum SDK version. However, we usually iterate much faster so we have to update the SDK manually by updating global.json.

SteveSandersonMS commented 5 years ago

It's pretty troubling to see that tests have been disabled in master for 24 days without teams being specifically notified. Did an email announcement go out that I missed? Engineering and merge decisions are often made based on whether we see test failures in CI.

In the past we've been burned badly by tests just being turned off, and getting stuck in a place where it's really hard to get them back on. When this is done, I'd really like to see someone directly reach out to affected teams and get a positive acknowledgement. Will that be possible in the future?

cc @Pilchie

JunTaoLuo commented 5 years ago

Addressed by @javiercn in https://github.com/aspnet/AspNetCore/pull/14101.

dougbu commented 5 years ago

'release/3.1' ProjectTemplate differs from 'master' in ways worth investigating:

TemplateTests.props.in in 'release/3.1' sets $(BundledNETCorePlatformsPackageVersion)
- i.e. f676c249d25e both disabled these tests in 'master' and changed them slightly
SpaTemplateTestBase in 'release/3.1' skips npm run test unless template == "react" || template == "reactredux"
- i.e. c53e177ea369 is only in 'master'
Directory.Build.props.in in 'master' sets $(TargetFramework) but that should be fine

dougbu commented 5 years ago

At this point, only a few template tests are skipped in 'release/3.1' and 'master'. That's tracked in #14036

'release/3.0' branch completely skips template tests since the last commit in #14003 (fa818fe1a1e5 though it won't exist for long since the PR was squashed). #14048 re-enables them.

Leaving this open to track merging the 3.0 fix.

dougbu commented 5 years ago

14048 is now done (see 06ca2c4ee3cc)