Skip to content

Restore retry on integration tests#48812

Merged
jaredpar merged 2 commits intodotnet:masterfrom
jaredpar:retry
Oct 21, 2020
Merged

Restore retry on integration tests#48812
jaredpar merged 2 commits intodotnet:masterfrom
jaredpar:retry

Conversation

@jaredpar
Copy link
Member

The retry mechanism on integration tests is predicated on not having the
--html option passed to RunTests. PR #48686 changed it to be
unconditionally passed which ended up breaking retry. Undoing the
unconditional passing of --html to fix retry.

The retry mechanism on integration tests is predicated on not having the
`--html` option passed to RunTests. PR dotnet#48686 changed it to be
unconditionally passed which ended up breaking retry. Undoing the
unconditional passing of `--html` to fix retry.
@jaredpar jaredpar requested a review from a team as a code owner October 21, 2020 06:16
Copy link
Contributor

@sharwell sharwell left a comment

Choose a reason for hiding this comment

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

This change restores retry for ci builds (good), but also enables it for local builds that run integration tests (bad). The logic doesn't need to rely on --html but should still switch on the local/ci condition somehow.

@jaredpar
Copy link
Member Author

At this point it's unclear to me what changed, but it seems clear to you. Can u please elaborate?

Having the IncludeHtml in the retry logic frankly seems like a bad idea. That is a very indirect indication of what is happening in our build.

@sharwell
Copy link
Contributor

The retry logic knows how to merge XML files to produce what looks like a single complete passing test run.

  • Everything that didn't fail is pulled from the first run
  • Everything else is pulled from the second run

It can't do the same for HTML results. Since it didn't seem necessary to automatically retry during local test runs, the easiest way to address this was skipping retry for local runs rather than implementing support for HTML result merging.

@jaredpar
Copy link
Member Author

I think having the retry be an implicit function of --testVsi is a mistake. Particularly because it is silently disabled when combined with --html. Given that retry is an explicit behavior it should be a separate option. One that can be combined with --testVsi and also lead to explicit errors when mixed with --html where it is not supported.

@sharwell
Copy link
Contributor

That absolutely works for me.

@ghost
Copy link

ghost commented Oct 21, 2020

Hello @jaredpar!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@jaredpar
Copy link
Member Author

Integration test appears to be a flaky failure. Hitting a lot of builds: five in last 24 hours.

https://runfo.azurewebsites.net/search/tests/?bq=definition%3Aroslyn-integration-ci++started%3A%7E1+&tq=message%3A%22Expected+a+light+bulb+session+to+appear%22

@jaredpar jaredpar merged commit ae8b081 into dotnet:master Oct 21, 2020
@ghost ghost added this to the Next milestone Oct 21, 2020
@jaredpar jaredpar deleted the retry branch October 21, 2020 23:06
@allisonchou allisonchou modified the milestones: Next, 16.9.P2 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants