Skip to content

E2E test fixes #35414

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 18 commits into from
Aug 18, 2021
Merged

E2E test fixes #35414

merged 18 commits into from
Aug 18, 2021

Conversation

SteveSandersonMS
Copy link
Member

As of this PR:

  • The Ignitor tests are all skipped.
    • Previously, they were already quarantined, but still ran and failed locally. As previously discussed on the team, we should remove both the Ignitor tests and Ignitor itself, but that can be done in a later PR.
  • Every other E2E test now passes, at least if you re-run enough times
    • Previously, there were a number of quarantined tests that would never pass due to bugs in either the test implementation or (in several cases) the recent string interpolation bug in .NET Core itself. These are all now fixed/workarounded, so I've seen them all go green at least once.

This doesn't address the reliability/consistency concerns, which we still need to take on as a later piece of work. Running the tests locally I don't see many (or any?) random failures of the form "expected x, got y" but I do see random failures like "the test server did not start". This might be assuaged by running without parallelism but it's a whole separate investigation.

@SteveSandersonMS SteveSandersonMS requested review from Pilchie and a team as code owners August 17, 2021 13:25
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for taking on this.

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Aug 18, 2021

Running locally now:

image

This includes running the quarantined tests. The skipped ones are the ignitor ones as described above.

@SteveSandersonMS
Copy link
Member Author

Thanks for approving, @javiercn and @TanayParikh. I was a bit premature in requesting review on this PR, since I needed to make a bunch of other small changes before it was ready. If either of you wants to review the changes since you saw it, please go ahead, but no need to if you don't feel like it because the changes are pretty minor, test-only, and I think noncontroversial.

@SteveSandersonMS SteveSandersonMS enabled auto-merge (squash) August 18, 2021 15:04
@SteveSandersonMS
Copy link
Member Author

Also just for the record, I've run the E2E tests 5 times in a row locally this afternoon, and all 5 times got a 100% pass rate.

I'm going to keep running these tests on a loop to collect more data, but at this point it looks like remaining reliability issues are probably CI-specific and not inherent to these tests.

@SteveSandersonMS SteveSandersonMS merged commit 10e3fe3 into main Aug 18, 2021
@SteveSandersonMS SteveSandersonMS deleted the stevesa/e2e-test-fixes-aug-2021 branch August 18, 2021 16:44
@ghost ghost added this to the 6.0-rc1 milestone Aug 18, 2021
@SteveSandersonMS
Copy link
Member Author

/backport to release/6.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/6.0-rc1: https://github.com/dotnet/aspnetcore/actions/runs/1143942181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants