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

Add a cecil test to compare [Availability] attributes (legacy) with [SupportedOSPlatform] (net6) #12544

Closed
spouliot opened this issue Aug 25, 2021 · 1 comment
Assignees
Labels
dotnet An issue or pull request related to .NET (6) dotnet-pri3 .NET 6: wishlist for stable release enhancement The issue or pull request is an enhancement
Milestone

Comments

@spouliot
Copy link
Contributor

Original discussion
https://discord.com/channels/732297728826277939/732297808148824115/879993217762857010

Background

  • net5 introduced new custom attributes for API availability
  • Our binding generator is automagically converting the legacy, [Availability]-based, attributes into [SupportedOSPlatform]
  • Not everything we ship is generated, we have manual bindings (e.g. p/invokes and helpers) that needs to have their attributes updated.
  • We have several introspection tests to validate [Availability]-based attributes.

Problem

  • Manual bindings updates are done manually, with some helpers (macros). This has potential for adding some mistakes, even if each PR is reviewed by one (or often many) peer(s).
  • Our tests, even if executed on older devices/simulators, cannot cover 100% of the cases, e.g. a cases like [iOS (12,3)] on an API that was added in [iOS (12,1)] would not be spotted by runtime test checks.

Potential Solutions

The real question is when to do something...

  • now
  • later... or
  • never

Do nothing

Why ?

  • Most API annotations are generated.
  • Most existing API annotations are correct
    • where correct means it match Apple documentation or, since it's often incorrect, is based on runtime tests
    • incorrect annotations that pass our existing tests will remain (incorrect)
  • Most of the PR updates are/will be correct
    • running intro on older devices/simulators will spot many (but not all) type of errors
  • Updating the attributes is not a breaking change (source or binary) if/when mistakes are found.

Add New Unit Tests

Each set of attributes, legacy and dotnet, exists in separate assemblies. Comparing them requires loading both so that would make it (easier to be a) cecil-based test.

Still this will not be perfect. This will only ensure that the legacy attributes are a match with the new net6 attributes (IOW GIGO).

Lifetime: This will be useful as long as legacy is supported.

@spouliot spouliot added enhancement The issue or pull request is an enhancement dotnet An issue or pull request related to .NET (6) dotnet-pri3 .NET 6: wishlist for stable release labels Aug 25, 2021
@spouliot spouliot added this to the .NET 6 milestone Aug 25, 2021
@chamons
Copy link
Contributor

chamons commented Apr 29, 2022

I believe this is effectively covered by the tests in

I don't actually compare legacy to NET6, but apply validation logic that even caught a significant number of incorrect legacy binding attributes.

As I continue testing on #10170 and the cleanup issues in #14802 I will add additional tests as I'm able.

@chamons chamons closed this as completed Apr 29, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dotnet An issue or pull request related to .NET (6) dotnet-pri3 .NET 6: wishlist for stable release enhancement The issue or pull request is an enhancement
Projects
None yet
Development

No branches or pull requests

2 participants