-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Remove reliance on "questionable" UpdateFile operation #52441
Conversation
What is making UpdateFile unreliable? Should we remove it? |
No idea. It's just a guess based on logs like this:
I wanted to remove it, but it's still used by another test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess if we think this is unsafe to use, then we should remove it and update tests. Or we don't have the evidence this is actually problematic then we should clarify the PR title accordingly.
@jasonmalinowski The code is used in three locations. This is the only location where tests in the last 30 days failed to achieve the desired outcome following the call. The issue has not been locally reproducible, and we have no evidence suggesting the reason why this call fails at this location. This pull request converts an operation which does not always produce the expected results (unreliable) to another form used elsewhere that does appear to produce the expected results. |
@jasonmalinowski I believe this addresses some of the issues reported in #52409 |
During investigation of #52409, logs were observed where the editor text did not appear to match the text set up by
InitializeAsync
(e.g. #52441 (comment) below). Since theInitializeAsync
method set the text using a somewhat unique method, and other tests aren't showing signs of incorrect values, we modify the problematic test to set the editor text using the same coding pattern that other non-problematic tests are using.The failure has not been locally reproducible, so it's not clear why (or to some degree if) the
UpdateFile
call is failing in this scenario.The new code follows the approach of
AbstractEditorTest.SetUpEditor
.Closes #52386