Skip to content
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

Skip flaky CommandLine.ArgumentParsing test #58100

Merged
merged 3 commits into from
Dec 3, 2021
Merged

Conversation

JoeRobich
Copy link
Member

No description provided.

@JoeRobich JoeRobich requested a review from a team as a code owner December 3, 2021 17:56
@@ -1410,7 +1410,7 @@ public void ModuleManifest()
Assert.Equal("blah", args.Win32Manifest);
}

[Fact]
[Fact(Skip = "https://github.com/dotnet/roslyn/issues/58077")]
Copy link
Member

Choose a reason for hiding this comment

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

@jaredpar pretty bad to lose all the validation of this method. should we comment out that portion taht is failing instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think it is better to disable specific problematic portion. Perhaps only on Linux DEBUG.

Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

I commented on the issue with a possible scenario that may be causing the failure. Instead of skipping I'd add a catch to the try/finally block, and re-throw the exception (possibly only in DEBUG). That way, we can better understand what's happening the next time it fails.

What do you think of that?

@JoeRobich
Copy link
Member Author

What do you think of that?

I believe we would certainly prefer to take a fix than skip this test.

@CyrusNajmabadi @jaredpar Thoughts about Youssef's suggestion?

@333fred
Copy link
Member

333fred commented Dec 3, 2021

I commented on the issue with a possible scenario that may be causing the failure. Instead of skipping I'd add a catch to the try/finally block, and re-throw the exception (possibly only in DEBUG). That way, we can better understand what's happening the next time it fails.

That can be done in a separate PR for investigation purposes. This test is blocking merging ability on a number of open PRs, and we need to unblock the pipeline.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 2)

@333fred 333fred enabled auto-merge (squash) December 3, 2021 18:20
@AlekseyTs
Copy link
Contributor

@JoeRobich Please include reference to the tracking issue into the commit description once you merge.

@jaredpar
Copy link
Member

jaredpar commented Dec 3, 2021

Given @Youssef1313 has a good lead on the underlying problem I would just take the most expedient step to disabling the test. Odds are we can fix and re-enable by EOD

@Youssef1313
Copy link
Member

Given @Youssef1313 has a good lead on the underlying problem I would just take the most expedient step to disabling the test. Odds are we can fix and re-enable by EOD

I doubt it will be by end of the day 😄
My guess seems to be wrong. I'll try to re-investigate next week after my mid-terms in case it wasn't picked by someone from the team.

@AlekseyTs
Copy link
Contributor

@JoeRobich The interesting CI jobs have passed. We probably don't want to wait another hour or so to get the integration tests green (as it stands right now, we need to re-run at least one leg). I think we should just force merge this PR, squashing the commits in the process, if possible. Note, I pushed some formatting changes to this PR to fix CI failures, those should be included.

@JoeRobich JoeRobich disabled auto-merge December 3, 2021 22:09
@JoeRobich JoeRobich merged commit 0b2c30d into main Dec 3, 2021
@ghost ghost added this to the Next milestone Dec 3, 2021
@JoeRobich
Copy link
Member Author

@AlekseyTs Squashed and merged.

@333fred 333fred deleted the dev/jorobich/skip-test branch December 3, 2021 22:19
JoeRobich added a commit that referenced this pull request Dec 6, 2021
* Remove IDE0051 suppression

* Add LayoutKind

* LayoutKind.Sequential

Co-authored-by: Sam Harwell <[email protected]>

* Create azure-pipelines-integration-dartlab.yml

* Update azure-pipelines-integration-dartlab.yml

* [Async lightbulb] Move suppression fixes to 'Low' priority bucket

Addresses second item in #56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.

* Cache last project and compilationWithAnalyzers

* Update test as normal priority bucket has one lesser code action

* Make shallow checkout optional for integration test CI

* Move pipeline checkout skips up a layer.

* Look up VS bootstrapper from build

* Revert skipPipelinesCheckout change

* Comment out checkout: none for now

* Added matrix

* Target topic branch in DartLab.Templates

* Update number of machines to 4

* Run test agent elevated

* Update pipeline description

* Default VS branch to main

* Address feedback

* Exclude unnecessary sqlite assemblies

* Convert namespace to file scoped when typing semicolon

* Working on tests

* Add tests

* Add comments

* Add comments

* Revert

* Revert

* Make static

* Add tests

* Pass AnalysisKind instead of int

* Honor option, and also improve formatting with comment

* Fix comment

* Add tests

* Fix await completion for expression body lambda

* Add new parser/lexer to the StackTraceAnalyzer (#57598)

No functional changes, just moving to the new API and cleaning up unused code

* Add comments

* Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (#57576)

Fixes #57577.

* Snap 17.1 P2 (#58041)

* Add new parser/lexer to the StackTraceAnalyzer (#57598) (#58050)

No functional changes, just moving to the new API and cleaning up unused code

* Read SourceLink info and call service to retrieve source from there (#57978)

* Merge pull request #58100 from dotnet/dev/jorobich/skip-test

Skip flaky CommandLine.ArgumentParsing test

See #58077 for more details

Co-authored-by: Youssef Victor <[email protected]>
Co-authored-by: Sam Harwell <[email protected]>
Co-authored-by: Brad White <[email protected]>
Co-authored-by: Manish Vasani <[email protected]>
Co-authored-by: Joey Robichaud <[email protected]>
Co-authored-by: Brad White <[email protected]>
Co-authored-by: [email protected] <[email protected]>
Co-authored-by: Cyrus Najmabadi <[email protected]>
Co-authored-by: Sam Harwell <[email protected]>
Co-authored-by: Andrew Hall <[email protected]>
Co-authored-by: Bernd Baumanns <[email protected]>
Co-authored-by: Allison Chou <[email protected]>
Co-authored-by: CyrusNajmabadi <[email protected]>
Co-authored-by: David Wengier <[email protected]>
Co-authored-by: Gen Lu <[email protected]>
333fred added a commit to 333fred/roslyn that referenced this pull request Dec 6, 2021
…rovements

* upstream/main: (68 commits)
  Lazy load ISourceLinkService to reduce DLL loads (dotnet#58108)
  [main] Update dependencies from dotnet/source-build (dotnet#57707)
  [main] Update dependencies from dotnet/arcade (dotnet#57968)
  Factor nullability logic for placeholders (dotnet#58036)
  Standardize list pattern lowering on `Index` constructor. (dotnet#58055)
  Add scripts to verify if a branch is ready to review
  Merge pull request dotnet#58100 from dotnet/dev/jorobich/skip-test
  Fix some places we weren't correctly disposing of VisualStudioAnalyzers
  Fix analyzer references being removed and added in one batch
  Fix indenting
  Ensure we don't silently capture any exceptions
  Don't MEF import the implementation directly if the public type will do
  Change comment
  Add comment
  Use actual jump tables
  Remove unused function
  Revert
  Simplify code
  Compute kind on demand
  Reorder
  ...
@Cosifne Cosifne removed this from the Next milestone Jan 5, 2022
@Cosifne Cosifne added this to the 17.1.P3 milestone Jan 5, 2022
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.

8 participants