Skip to content

Moving common logic of keyboard's simulation into the helper class. #7537

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

Conversation

NikitaSemenovAkvelon
Copy link
Contributor

@NikitaSemenovAkvelon NikitaSemenovAkvelon commented Aug 4, 2022

Fixes #7506

Proposed changes

  • Added KeyboardHelper class in System.Windows.Forms.TestUtilities project.
  • Used this class instead of copy-pasted code in unit tests.

Customer Impact

  • It's a technical refactoring.

Regression?

  • No

Risk

  • Minimal

Test methodology

  • Unit tests

Test environment(s)

  • 7.0.100-preview.5.22307.18
Microsoft Reviewers: Open in CodeFlow

@Tanya-Solyanik
Copy link
Contributor

Tanya-Solyanik commented Aug 4, 2022

Moving to .NET8 because this is a refactoring and does not meet the bar for .net7

Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

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

Looks good, I added a few nits. Please don't merge until main becomes a NET8 branch

@Tanya-Solyanik Tanya-Solyanik added waiting-author-feedback The team requires more information from the author 🚫 * NO-MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) labels Aug 4, 2022
@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_7506-Refactoring_KeyboardHelper branch from d4f85a9 to ebb65a8 Compare August 5, 2022 08:51
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Aug 5, 2022
@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_7506-Refactoring_KeyboardHelper branch from ebb65a8 to 246e199 Compare August 5, 2022 08:53
Tanya-Solyanik
Tanya-Solyanik previously approved these changes Aug 8, 2022
Copy link
Contributor

@vladimir-krestov vladimir-krestov left a comment

Choose a reason for hiding this comment

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

Looks good! ✅

// https://docs.microsoft.com/windows/win32/inputdev/wm-keyup
// The MSDN page tells us what bits of lParam to use for each of the parameters.
// All we need to do is some bit shifting to assemble lParam
// lParam = repeatCount | (scanCode << 16)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is different with the code below. So, I would just remove this line. We see lParam below.

Copy link
Contributor

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

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

Please replace ToInt with FromLowHighUnsigned

@Tanya-Solyanik Tanya-Solyanik self-requested a review August 8, 2022 22:31
@Tanya-Solyanik Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Aug 8, 2022
Copy link
Contributor

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

🚀

@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_7506-Refactoring_KeyboardHelper branch from 246e199 to 0b23ab7 Compare August 9, 2022 04:13
@ghost ghost removed the waiting-author-feedback The team requires more information from the author label Aug 9, 2022
Tanya-Solyanik
Tanya-Solyanik previously approved these changes Aug 9, 2022
@vladimir-krestov
Copy link
Contributor

Please rebase to the latest main to resolve build errors

@NikitaSemenovAkvelon NikitaSemenovAkvelon force-pushed the Issue_7506-Refactoring_KeyboardHelper branch from 0b23ab7 to a63940c Compare August 9, 2022 10:48
@Tanya-Solyanik
Copy link
Contributor

Please rebase to the latest main to resolve build errors

@vladimir-krestov - you should be able to re-run build by adding a azp /run comment to the PR. Could you please double-check if this works for you when you have a chance?

@RussKie
Copy link
Contributor

RussKie commented Aug 10, 2022

@Tanya-Solyanik this change only affects the test infra, is there any reason not to have this merged?

@Tanya-Solyanik
Copy link
Contributor

@Tanya-Solyanik this change only affects the test infra, is there any reason not to have this merged?

good point

@Tanya-Solyanik Tanya-Solyanik removed the 🚫 * NO-MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 11, 2022
@Tanya-Solyanik Tanya-Solyanik modified the milestones: .NET 8.0, .NET 7.0 Aug 11, 2022
@Tanya-Solyanik Tanya-Solyanik merged commit 89924dc into dotnet:main Aug 11, 2022
@ghost ghost modified the milestones: .NET 7.0, 7.0 RC1 Aug 11, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework unit test infra for keyboard keypress simulations
4 participants