Skip to content

Revert "Read syntax tree, source text and options synchronously in command handlers"#62189

Merged
RikkiGibson merged 1 commit intomainfrom
revert-62002-SynchHandlers
Jun 28, 2022
Merged

Revert "Read syntax tree, source text and options synchronously in command handlers"#62189
RikkiGibson merged 1 commit intomainfrom
revert-62002-SynchHandlers

Conversation

@RikkiGibson
Copy link
Member

@RikkiGibson RikkiGibson commented Jun 28, 2022

#62002 is suspected of introducing required test failures.

Last known good: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/407545#view=discussion
First known bad: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/407578#view=discussion
Revert validation: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=6344859&view=results

Failing tests:

DD-VS-PR-DDRIT: VBWebSiteTests Email owner Assert.Fail failed. (1:08.581) [Platform:Testcase] Verification failed: String content is equal: Expected ' Dim s As String', Actual ' Dim s As String'; <ComparisonType: OrdinalIgnoreCase>

Please advise whether the failures are plausibly connected with this PR, and if so, whether we should revert, or take a follow-up PR to fix, or adjust the test baseline on the VS side. @chsienki @tmat @CyrusNajmabadi.

@RikkiGibson RikkiGibson requested a review from a team as a code owner June 28, 2022 17:57
@RikkiGibson RikkiGibson requested a review from a team June 28, 2022 17:57
@ghost ghost added the Area-IDE label Jun 28, 2022
@jasonmalinowski
Copy link
Member

@RikkiGibson: are you able to clarify what's different between that "expected" and "actual" on the test there?

@jasonmalinowski
Copy link
Member

Ah, found the diff:

Expected: '            Dim s As String',
Actual:   '        Dim s As String'

Rollback may make sense, since the PR changed how we are handling formatting options.

@RikkiGibson
Copy link
Member Author

RikkiGibson commented Jun 28, 2022

No, but more detail should be available in the internal links. Most likely, there's a difference in indentation which isn't showing up (maybe due to round-tripping the result text through HTML?)

edit: too slow! 😄

@tmat
Copy link
Member

tmat commented Jun 28, 2022

Sounds good. Seems like we don't have a test covering this case :(

@RikkiGibson RikkiGibson merged commit 791de28 into main Jun 28, 2022
@ghost ghost added this to the Next milestone Jun 28, 2022
@RikkiGibson RikkiGibson deleted the revert-62002-SynchHandlers branch June 28, 2022 19:53
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
tmat added a commit that referenced this pull request Jun 28, 2022
tmat added a commit to tmat/roslyn that referenced this pull request Jun 28, 2022
tmat added a commit that referenced this pull request Jun 30, 2022
* Revert "Revert "Read syntax tree, source text and options synchronously in command handlers (#62002)" (#62189)"

This reverts commit 791de28.

* Read SmartIndent directly from global options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants