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

Clean up tests under Interop/PInvoke/BestFitMapping #61390

Merged
merged 5 commits into from
Nov 11, 2021

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Nov 10, 2021

  • Collapse a bunch of different test projects targeting BestFitMapping
  • Pull out shared code for test data and validation
  • Add test cases specifically for unmappable characters (they were previously grouped with characters that did have a best-fit mapping and were commented out for some cases)

Aside from making my eyes bleed less when I look inside that folder, this reduces the time it takes to run the tests, time to upload results, and size of the uploaded test artifact for each PR:

Before After
Number of projects in Interop/PInvoke/BestFitMapping 48 5
Interop.PInvoke.XUnitWrapper test run (windows x64 checked) ~23s ~12s
Uploading files for Interop/PInvoke/BestFitMapping results ~14s ~1s
CoreCLRManagedTestArtifacts_AnyOS_AnyCPU_checked.zip size 47.8 MB 46.5 MB

@AaronRobinsonMSFT @jkoritzinsky - sorry, the diff is probably not very helpful... All the p/invoke definitions were direct copies/moved files. The existing methods for validation were nearly all the same with assorted discrepancies where some things were or were not checked, so I pulled that all out into a shared helper so that they would avoid said discrepancies without changing how/what they were validating.

@elinor-fung elinor-fung changed the title Clean up on consolidate tests under Interop/PInvoke/BestFitMapping Clean up tests under Interop/PInvoke/BestFitMapping Nov 10, 2021
@elinor-fung elinor-fung marked this pull request as ready for review November 10, 2021 23:05
@elinor-fung elinor-fung added this to the 7.0.0 milestone Nov 10, 2021
@elinor-fung
Copy link
Member Author

cc @trylek - this change seems like a manual version of the test grouping you and @jkoritzinsky are working on and I think it further reinforces how much value that work would bring.

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

Successfully merging this pull request may close these issues.

3 participants