Add public API approval tests for Rx.NET

[ryanwersal]

For the sake of completeness, I stole the ReactiveUI PR template:

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature.

What is the current behavior? (You can also link to an open issue here)
No public API approval tests for projects under the Rx.NET folder. #479

What is the new behavior (if this is a feature change)?
Public API approval tests are added for the projects under the Rx.NET folder.

What might this PR break?
Unsure. I ended up electing to add a new project for the API approvals to reduce the amount of hacking up the existing projects required (due to PublicApiGenerator and ApprovalTests having limitations with regards to netcoreapp targets) so there is a risk of CI breakage.

Please check if the PR fulfills these requirements

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:
Fixed. Hack removed and version bumped.

I found a bug in PublicApiGenerator that I have a PR upstream to fix. In the meantime, I kept the PackageReference at the current version and added a hacked up version of ApiGenerator which contains the fix as a stopgap. I'll submit a new PR with a package version bump and removal of the hacky code once upstream PublicApiGenerator merges the PR on their end.

Additionally, if this largely checks out I'll wrap up the same work for the projects under the Ix.NET folder (specifically the hacked up ApiGenerator file and the additional project).

Apologies for the delay in getting this submitted, I ended up needing to learn and investigate substantially more than I had originally anticipated. It was a good learning experience though! 👍

[dnfclas]

All CLA requirements met.

[danielcweber]

Thanks, good work. Approval tests for System.Reactive should probably be added because in contrast to 3.x, it is no longer just a meta package but contains actual code and thus, a public API. System.Reactive.Core however has approval tests but is just a facade, I don't know whether facades need approval tests as well. Besides from that, once that's sorted out, looks ok to merge.

Edit: Sorry my mistake, although the test is called "Core", the checked assembly is indeed, System.Reactive. So from my perspective, this is good to go.

[ryanwersal]

Thanks for the review! I'm open to changing the "Core" test name to something that would be clearer about its intent. I admittedly struggled with it a bit while putting the tests together. I can also remove the facade approvals if that is desirable as I wasn't sure how much verification we would want for those projects.

[danielcweber]

IMO the facade approvals should be kept, they require API stability as well.

[danielcweber]

Apart from a possible rebase onto master, this is absolutely fine.

[danielcweber]

@onovotny Should be good to go for 4.0 stable as well.

[danielcweber]

@ryanwersal Can you please rebase, it'll be merged then.

[danielcweber]

@ryanwersal This needs a small update since the Interfaces-project is now only in facades and there is a merge conflict with the solution. I'll happily merge it.

[ryanwersal]

@danielcweber I'll be able to get this rebased on latest master later today.

[ryanwersal]

@danielcweber I pushed a squashed rebase with a couple fixes. Notably:

• I found a couple issues with my build script changes; the latest changes should correctly handle errors like the other test commands and respect configuration specifications from environment variables.
• I removed the Interfaces approvals since most of them were picked up by the Core test and the other facade projects didn't have approvals.
• I also updated the approvals to reflect current head of master (using net46 instead of net45 etc).

Thanks!

[danielcweber]

Awesome, thanks. Now we have to make those test run in CI somehow....they should be run by the build-script, right? Currently, they are not.

[danielcweber]

Ok, they are run, and successfully, they are just not reported to VSTS.

[danielcweber]

I will merge it for now to have the tests included for safer future merges, maybe you can get the results to be reported to VSTS, if you need assistance with the VSTS build configuration, @onovotny will be happy to help you. Thx!