Enabled Baseline Package Validation

[304NotModified]

see #3161 (comment)

• Enabled Package baseline validation, see https://learn.microsoft.com/en-us/dotnet/fundamentals/apicompat/package-validation/baseline-version-validator
• Use 1.52.0 as baseline (as discussed below)
• Added CompatibilitySuppressions.xml for breaking changes sinds 1.52 (discussed below)
• CLI package is not in scope
• Added CP0006 - A member was added to an interface without a default implementation (see see https://learn.microsoft.com/en-us/dotnet/fundamentals/apicompat/diagnostic-ids) to <NoWarn> on Playwright.csproj, as that is the normal flow?
• Add a new workflow for checking the package results. Another option is to add <GeneratePackageOnBuild>True</GeneratePackageOnBuild> to the csproj files
  • "NuGet Package Smoke Test" is not testing all the packages and have a different goal
• Example of breakage: Package baseline validation (breaking example) #3170

[mxschmitt]

It looks great but I'm asking myself if its worth using a very old version as a baseline. I'd instead use a more recent one and then continue the journey from there?

[304NotModified]

As we do semver, we would like to know the breaking changes when someone upgrades from a old version?

Some breaking changes are acceptable, and those are in the suppression file :)

[304NotModified]

It seems we have already new breaking changes and not sure if they are intentional 😅

[304NotModified]

@mxschmitt
So this is the new breaking change in master (introduced by d0f2745)

First we had a public empty ctor for BindingSource.cs, now that ctor is gone!

Is that intentional?

Anyway, shown by the baseline package validation :)

I've added it to the suppressions by dotnet pack /p:ApiCompatGenerateSuppressionFile=true

[mxschmitt]

First we had a public empty ctor for BindingSource.cs, now that ctor is gone!

Yes, because BindingSource is a class we construct and not the user.

I think lets consider everything before the latest release as "it was intentional" instead of committing a bunch of irrelevant exceptions to the repository. Do you mind adjusting it? then we can merge.

[304NotModified]

So the baseline version should 1.52.0, correct?

[mxschmitt]

So the baseline version should 1.52.0, correct?

Yes

[304NotModified]

OK working on it :)

[304NotModified]

@mxschmitt

Updated, but there is also a new (small) breaking change?

From #3184 - the Ref property is removed from LocatorAriaSnapshotOptions.cs

But it's documented a new in 1.52, so it just introduced.

https://playwright.dev/dotnet/docs/api/class-locator#locator-aria-snapshot

Anyway, if this in indented, then this PR is ready

[mxschmitt]

Awesome! Yeah the Ref property was intentionally removed.

[304NotModified]

All green. Feel free to rename the PR before merge :)

[kblok]

@304NotModified @mxschmitt are we semver? I think playwright broke compatibility many times.

[304NotModified]

@kblok

I hope this PR will help in getting the API more stable in terms of breaking changes.
Sometime there are good reasons to break compatibility, this is now more transparent and explicit.

My report of the binary breaking change (#3161) was quickly fixed, so thanks for that! I hope we have less need for these fixes in the future :)

[kblok]

@304NotModified The problem is that upstream playwright determines the API, andplaywright-dotnet matches the upstream version. If upstream breaks the compatibility without changing the major version, there is nothing we can do here.

[304NotModified]

@304NotModified The problem is that upstream playwright determines the API, andplaywright-dotnet matches the upstream version. If upstream breaks the compatibility without changing the major version, there is nothing we can do here.

@kblok
I don't think that's always true. E.g. instead of removing the Ref property (see #3169 (comment)) we could mark it with a [Obsolete] attribuut. Now it's a binary and source breaking change.