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

Fix/skip integration tests #10959

Merged
merged 7 commits into from
Oct 4, 2024
Merged

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Oct 3, 2024

We have 9 failing integration tests, seemingly because the test machines are picking up FUSE. This PR skips some (with links to bugs) or works around the issues.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2268945

@davidwengier davidwengier requested a review from a team as a code owner October 3, 2024 06:56
Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

Classifications seem to have changed 🤷‍♂️

Interesting! I would expect "class name" to come from Roslyn semantic ranges and "RazorTagHelperElement" to come from Razor. I know you said everything looked OK, but it worries me that "class name" never shows up. Does that mean that this is actually a regression and tag helper elements are no longer colorized using the "User Types - Classes" Fonts and Colors setting? Instead of updating base lines in integration tests with unexpected results, is there a deeper regression that should be investigated?

@davidwengier
Copy link
Contributor Author

davidwengier commented Oct 3, 2024

Instead of updating base lines in integration tests with unexpected results, is there a deeper regression that should be investigated?

Yes, and since opening the PR I believe this is likely a FUSE-related regression.

The reason I think it's worth changing the baseline for these specific tests, which I definitely should have mentioned in the description, is because the classification in question here is largely unrelated to the tests. The tests are specifically validating project load when a Razor buffer is created early in the startup lifecycle, and the fact that they both failed caused me to start investigating that path, which is long and complicated. Turns out, it's just that these are the only two integration tests that wait for the "class name" classification 🤦‍♂️

By changing the tests to wait for a Razor defined classification we should be more immune to changes, either due to Roslyn or to the generated code, in these specific tests. There are existing tests that validate classifications are as we expect, and I plan on adding coverage to them for FUSE. I had thought that was already done somewhere but my hunch now is that it's not.

Does that mean that this is actually a regression and tag helper elements

Ironically the opposite. Tag helpers still appear purple (for me and whatever theme I have), and this change validates that moving forward. The "class name" classification would have applied to the text after the @model directive which is where I suspect FUSE is not providing source mappings.

@davidwengier
Copy link
Contributor Author

Okay, I have found our test that covers semantic tokens in @model and it turns out its a bad test, and doesn't fail under FUSE 😔

@davidwengier
Copy link
Contributor Author

Pushed up test changes so our semantic tokens tests now cover @model correctly, logged #10963 for the FUSE bug, and verified that these new tests fail when FUSE is turned on.

@davidwengier davidwengier changed the title Fix integration tests Fix/skip integration tests Oct 3, 2024
@davidwengier
Copy link
Contributor Author

Skipped a couple more tests that already have FUSE bugs logged.

@ryzngard
Copy link
Contributor

ryzngard commented Oct 3, 2024

We have 9 failing integration tests, seemingly because the test machines are picking up FUSE. This PR skips some (with links to bugs) or works around the issues.

Oof... I didn't think test machines got TNs. I wonder if there's a setting to disallow that...

@davidwengier
Copy link
Contributor Author

davidwengier commented Oct 4, 2024

eh, I don't think its a bad thing necessarily. These test failures essentially found https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2268945 before CTI did (not that we noticed or acted in time 😛)

Having control over feature flags would be good though, to get FUSE and non-FUSE runs, and that would have made it immediately obvious where FUSE was causing issues, but we've talked about that many many times.

@davidwengier
Copy link
Contributor Author

Integration tests: ✅

@davidwengier davidwengier merged commit 6943d77 into dotnet:main Oct 4, 2024
17 checks passed
@davidwengier davidwengier deleted the FixIntegrationTests branch October 4, 2024 21:54
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Oct 4, 2024
davidwengier added a commit that referenced this pull request Oct 8, 2024
Cherry pick of the commit from
#10959 that fixes code actions in
FUSE, **and fixing the generate event handler code action which is
unrelated to FUSE**, in case we decide we want to take it for 17.12 P3

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/2268945
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.

4 participants