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 exception dialog handling. #8695

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Conversation

JeremyKuhne
Copy link
Member

@JeremyKuhne JeremyKuhne commented Feb 24, 2023

Test fixtures do not run on the same thread the test method executes on when marked with [WinFormsFact] or [StaFact]. This change removes the test fixture and adds a new context switch to throw thread exceptions.

Initialization of the switch is done programmatically via a module initializer. The module initializer will also set the starting cursor position to avoid spurious errors in tests due to cursor positioning.

The Cursor tests were the ones modifying the cursor position and causing intermittent errors in other tests. Moved them to their own collection and forced them to run sequentially.

Cleaned up WindowsFormsSynchronizationContext.

Simplified checks in LocalAppContextSwitches. We shouldn't be conditioning on supported OS beyond checking for API availability. We don't support Windows 7, but we won't deliberately cause WinForms apps to crash on Windows by calling later APIs without checking for support. We will not add additional complexity to the code beyond that for unsupported OS platforms.

Microsoft Reviewers: Open in CodeFlow

Test fixtures do not run on the same thread the test method executes on when marked with `[WinFormsFact]` or `[StaFact]`. This change removes the test fixture and adds a new context switch to throw thread exceptions.

Initialization of the switch is done programmatically via a module initializer. The module initializer will also set the starting cursor position to avoid spurious errors in tests due to cursor positioning.

The Cursor tests were the ones modifying the cursor position and causing intermittent errors in other tests. Moved them to their own collection and forced them to run sequentially.

Cleaned up WindowsFormsSynchronizationContext.

Simplified checks in LocalAppContextSwitches. We shouldn't be conditioning on supported OS beyond checking for UI availability. We don't support Windows 7, but we won't deliberately cause WinForms apps to crash on Windows by calling later APIs without checking for support. We will not add additional complexity to the code beyond that for unsupported OS platforms.
@JeremyKuhne JeremyKuhne requested a review from a team as a code owner February 24, 2023 20:57
@ghost ghost assigned JeremyKuhne Feb 24, 2023
@JeremyKuhne
Copy link
Member Author

Fixes: #8657

Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

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

🥳

@lonitra lonitra linked an issue Feb 24, 2023 that may be closed by this pull request
@Tanya-Solyanik
Copy link
Member

We shouldn't be conditioning on supported OS beyond checking for API availability.

This is pretty much the case of API availability. PMv2 is not available on these OSs, if the user app follows that path, hits the unavailable API, we don't have a way to handle it cleanly.

I would rather factor this change out of this PR and investigate it more.

@JeremyKuhne
Copy link
Member Author

This is pretty much the case of API availability. PMv2 is not available on these OSs, if the user app follows that path, hits the unavailable API, we don't have a way to handle it cleanly.

It should be done at the consuming site or clearly documented here ("this feature uses APINAME which requires OS blah").

@JeremyKuhne
Copy link
Member Author

I would rather factor this change out of this PR and investigate it more.

I'm out for a week and this is needed for additional test work so I'm not going to pull it out.

@JeremyKuhne JeremyKuhne merged commit 9749eb0 into dotnet:main Feb 24, 2023
@ghost ghost added this to the 8.0 Preview3 milestone Feb 24, 2023
@RussKie
Copy link
Member

RussKie commented Feb 27, 2023

Nice!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ThreadExceptionFixture Instantiated in Wrong Thread
4 participants