search

microsoft / axe-windows

Automated accessibility testing engine for Windows applications
MIT License
136 stars 62 forks source link

Use CsWin32 to generate NativeMethods #792

Closed mcooley closed 1 year ago

mcooley commented 1 year ago

Details

Uses the CsWin32 library to generate P/Invoke wrappers for windows APIs that Axe.Windows calls.

Motivation

Eliminate the need to maintain P/Invoke method definitions, since they will now be generated based on official metadata from the Windows team.

Context

Eventually, I would like CsWin32 to generate interop code not only for these methods but also for the whole UI automation API. Ideally the .NET ecosystem would not need to maintain libraries like UIAutomation-Interop just to wrap UIA. I've been using the Axe.Windows codebase to prototype this, and as a result I've submitted some fixes and suggestions upstream.

I'm submitting this PR to check whether Axe.Windows maintainers would be interested in using CsWin32 in general, before I go further to prepare a change to use CsWin32 for UIA.

codecov-commenter commented 1 year ago

Codecov Report

Merging #792 (3a241ca) into main (4fc19c3) will increase coverage by 1.14%. The diff coverage is 78.01%.

@@            Coverage Diff             @@
##             main     #792      +/-   ##
==========================================
+ Coverage   73.70%   74.84%   +1.14%     
==========================================
  Files         398      421      +23     
  Lines       12046    12987     +941     
==========================================
+ Hits         8878     9720     +842     
- Misses       3168     3267      +99
Impacted Files Coverage Δ
src/Actions/Actions/ControlPatternAction.cs 0.00% <0.00%> (ø)
src/Actions/Actions/CustomUIAAction.cs 0.00% <0.00%> (ø)
src/Actions/Actions/ListenAction.cs 0.00% <0.00%> (ø)
src/Actions/Actions/LoadActionParts.cs 90.90% <ø> (ø)
src/Actions/Actions/SetDataAction.cs 0.00% <0.00%> (ø)
...rc/Actions/Attributes/InteractionLevelAttribute.cs 0.00% <ø> (ø)
src/Actions/Contexts/ElementDataContext.cs 86.36% <ø> (ø)
src/Actions/Misc/ExtensionMethods.cs 7.46% <0.00%> (-31.60%) :arrow_down:
src/Actions/Resources/ErrorMessages.Designer.cs 0.00% <ø> (-55.56%) :arrow_down:
src/Automation/Data/OutputFile.cs 88.88% <ø> (ø)
... and 287 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dbjorge commented 1 year ago

I think we should probably get a few different folks' opinions on this (@DaveTryon / @RobGallo ?). My two cents:

Overall, I'm not opposed to using this project in concept, but I would feel more comfortable waiting to accept this dependency until the project declares a stable version and has demonstrated some stable bake time with it.

marcelwgn commented 1 year ago

If you don't mind me jumping in on this discussion as a consumer of Axe.Windows : I see the benefit of adding a library to get rid of code maintaining these interops from a maintainer standpoint, after all it's less code to maintain. However, reading that this library still has active bugs and still is shipping as pre-release only, as a consumer I'm a bit worried of having to try and figure out these issues when Axe is hitting these bugs. Especially since Axe is running against rendered UIs which can be flaky in CI, adding this additional possible layer of uncertainty is something I would prefer avoided. As @dbjorge pointed out, since those things are also not the easiest to cover properly with Unit Tests, this seems like a risk to stability of Axe.Windows.

mcooley commented 1 year ago

I'd like to contextualize "CsWin32 still has active bugs": the scope of CsWin32 is to generate code to call every Windows API. Because of that large scope, I think it will be a while before the project can meaningfully declare stability.

But Axe.Windows uses only a few Windows APIs, and the APIs it uses are not exotic, so I expect that the code generated by CsWin32 in this project will be stable in practice. Although I personally don't think it's necessary, I would understand if project maintainers say they want "officially stable" instead of "stable in practice"...just be aware that it may take a while.

DaveTryon commented 1 year ago

@mcooley We'll discuss this in our weekly triage meeting then get back to you.

DaveTryon commented 1 year ago

@mcooley We discussed this in our triage meeting and we don't feel that this is a good change for us at this time. The NativeMethods class may be old-school, but it never changes, is a known quantity, and works in all cases, including on Win7, which is currently our minimum required platform. Moving to CsWin32 won't improve the user experience, won't save us engineering time, and in a worse case scenario could introduce a bug that we might then have trouble tracing back to the root cause. Closing the PR.