Add API approvals to tests

[Shane32]

Summary

This pull request integrates API Approvals into our testing framework, designed to ensure that all changes to the public APIs of our projects are deliberate and documented. This feature automatically generates or updates a snapshot of the public API in a text document for each project and target framework during local tests. During continuous integration, these snapshots are verified to ensure they have been appropriately updated and included in the pull request.

Implementation Details

• Local Test Snapshot Generation/Update: During local development, running the tests will automatically generate or update the existing snapshot of the public API for each project and target framework. This ensures that developers can immediately see and verify any changes they make to the API.
• CI Verification: In the continuous integration environment, the API snapshots are checked to confirm that they match the API changes included in the pull request. If there is a mismatch, the CI build will fail, prompting for an update to the snapshots to reflect the latest API state.
• GitHub Integration: Updated API snapshots are included in pull requests, allowing GitHub to display the modified public API changes directly in the pull request interface. This visibility makes it easier for reviewers to understand the scope and impact of the API changes being introduced.

Benefits

• Immediate Feedback on API Changes: Developers get instant feedback on how their code changes affect the public API, enabling quick adjustments and informed decision-making.
• Streamlined Review Process: Including updated API snapshots in pull requests provides reviewers with a direct view of API changes, facilitating a more effective and focused review process.
• Enhanced Documentation: Automatic generation and updating of API snapshots helps ensure our documentation remains in sync with the actual codebase, aiding both new and existing users.
• Prevention of Unintended API Modifications: By enforcing snapshot updates and verifications, we safeguard against unintentional API alterations that could lead to compatibility issues for our users.

Conclusion

The addition of automated API Approvals and snapshot updates to our testing and integration processes significantly strengthens our control over the project's public interfaces. This integration ensures high standards of compatibility and reliability, shielding end-users from unintended changes and fostering a stable development environment.

[Shane32]

Well, the PR works ... sort-of. It won't pass tests because the CI build does not have SupportedOSPlatformAttribute on any members of any of the builds. If you import the 1.5.0 NuGet package into a new net6.0 project, and try using Base64QRCode.ImageType.Gif it does not have the attribute and will not provide a warning. (Just one example.)

So, while the PR accurately identifies that the build is broken ... the build is broken. I'm going to try modifying the build workflow until I can identify the problem.

[codebude]

So you say the recently published 1.5.0 nugget package is broken? Or did you mean the build for this specific PR was broken?
If 1.5.0 is broken, we should fix it asap and release a 1.5.1 before more people download a broken release.

[Shane32]

The recently published NuGet package 1.5.0 doesn’t have any SupportedOSPlatformAttribute markings in it. Not a critical issue, since mostly it was marked on windows-only members in the net6.0-windows build, but nevertheless, it is not correct.

I was able to fix the issue by using dotnet build instead of msbuild in the CI script. (We can see the test passes now.)

[Shane32]

I’d probably release 1.5.1 if it was me, but the only affect will be people on net6.0 trying to create a base64 QR code encoded as gif/jpeg. They would get a build warning before attempting it in production and getting an exception.

[codebude]

Thanks for your help. I will release 1.5.1 this evening (GMT+2 - don't know if it's your evening, too).

But I don't nevertheless I don't understand how msbuild could produce this error. I would understand it, if Ms build failed the build completely, but it built. There's was an artifact produced. I would have expected to see at least some errors. :-/

By the way - same old story. I chose msbuild over dotnet build for a reason, but can't remember the reason anymore. (Probably the reason is also overhauled over the years...)

[Shane32]

The 1.5.1 build is correct. Attached is a screen snip from a consuming project, which did not look this way with 1.5.0:

[Shane32]

@codebude Now that 1.5.1 is released, what are your feelings towards additional improvements? If you have the time to review additional PRs, I may have time to provide a few more PRs. Most specifically for v1.x, I would like to provide a performance update PR. For v2, I could assist with PR(s) to split off System.Drawing.Common functionality and bring SkiaSharp and ImageSharp to QRCoder. Finally, I may be able to assist with improvements with the CI scripts. For instance, currently all tests run on Windows; it would not be difficult to also run tests on Linux.

[codebude]

If you have the time to review additional PRs, I may have time to provide a few more PRs.

Taking the time for reviews is the least I can do if you're already devoting so much work/time to the QRCoder. So yes, gladly!

Most specifically for v1.x, I would like to provide a performance update PR.

That sounds like a great idea. Performance optimizations are always welcome.

For v2, I could assist with PR(s) to split off System.Drawing.Common functionality and bring SkiaSharp and ImageSharp to QRCoder.

Here I would ask you to wait a little bit. I have been thinking about version 2.0 for the last few days and have identified a few points that need to be clarified before we start with a 2.0. I am currently preparing a larger post here in which I would like to collect feedback from the community first.

Finally, I may be able to assist with improvements with the CI scripts. For instance, currently all tests run on Windows; it would not be difficult to also run tests on Linux.

This is also a great idea that can already be implemented now (before version 2.0). Additional tests under Linux would certainly not do any harm. It might also be a possibility to work on the performance of the CI/build scripts. The total execution time of 12-15 minutes is already very slow and sometimes painful... There may still be scope for runtime optimization here too.

[Shane32]

Sounds good. Look for a performance PR soon. So far it cuts down the memory allocation for small QR codes by about 50%, and large ones by about 30%. I'm only about halfway done though. It's going to change a lot of code, however, but not the structure (the method names are all the same, and the signatures change very little). I could just change a few methods per PR if you want it broken out more.

[Shane32]

Within the performance PR, in certain areas I may include all the old code as comments, so it's easier to see what was translated to what - if GitHub's compare is inadequate. Then we can delete the comments after you've had time to review the code.

[Shane32]

(no public API changes, of course)

[codebude]

Within the performance PR, in certain areas I may include all the old code as comments, so it's easier to see what was translated to what - if GitHub's compare is inadequate. Then we can delete the comments after you've had time to review the code.

That sounds like a plan. In general I think we've a very complete/robust set of test cases around the core generator so that even bigger code changes might be tested accordingly. Nevertheless I'll have a deeper look onto the changes and might pull out the ISO Norm I used back then when implementing the generator initially. ;-)