APIApprovals for Core

[danielmarbach]

Contributes to #1607

Here is a proposal which we at Particular Software use to make conscious decisions about API breaking changes. It doesn't address all the things raised by @Aaronontheweb but it is one step in the right direction. So basically the idea is the following:

• Uses ApiApprovals and ApprovalTests to generate a public API of a given assembly.
• The public API gets approved by a human into a *.approved.txt file.
• Everytime the API approval test runs the API is generated again into a *.received.txt file. If the two files don't match the test fails on the CI server or locally. Locally on the devs machine the predefined Diff viewer pops up (never happens on CI) and the dev has to approve the API changes (therefore making a conscious decision)
• Each PR making public API changes will contain the *.approved.txt file in the DIFF and all reviewers can easily see the breaking changes on the public API.

Below a few screenshots for better visualisation:

On local machine

Diff viewer pops up

Approve changes if consciously made

Diff

Thoughts? If you like it it can send in more for possibly:

• Persistence
• ?? please append

And then later address the additional ideas raised by @Aaronontheweb

But I think this PR would already be a huge win.

[danielmarbach]

So there is no build triggered for this PR? More see https://blog.jetbrains.com/teamcity/2013/02/automatically-building-pull-requests-from-github-with-teamcity/

[Aaronontheweb]

@danielmarbach build should have been triggered - we follow that process. I'll take a look

[danielmarbach]

What do you think about the PR itself?

Am 04.01.2016 um 18:21 schrieb Aaron Stannard notifications@github.com:

@danielmarbach build should have been triggered - we follow that process. I'll take a look

—
Reply to this email directly or view it on GitHub.

[Aaronontheweb]

@danielmarbach I'll take a look at it today - and I'm really impressed with how quickly you did this and how thorough you were in documenting your PR :)

[Aaronontheweb]

Why is this referenced 4 times in this .csproj file?

[Aaronontheweb]

Derp, nevermind - 4 different DLLs inside the same package. silly me.

[Aaronontheweb]

@rogeralsing I'm torn about including this inside Akka.Tests - think we should move it to its own project?

[rogeralsing]

As a separate project imo, that way, it could run as a separate build step on the CI engine. would make it much easier to spot the reason for the failure, if it is failures in the Akka code or API changes.

[Aaronontheweb]

@rogeralsing I agree with that

@danielmarbach could you move the API approval tests into their own project? Akka.APIApproval?

[Aaronontheweb]

Fixed the issue with the build server - apparently our Azure cloud service we use for auto-scaling build agents was permanently faulted by something that occurred at midnight 12/31. Switched to a new cloud service and everything is running correctly again.

[danielmarbach]

@rogeralsing @Aaronontheweb Thanks for the feedback. Please bear with me while I try to be the devils advocate here.

The "only" reasoning I heard so far is that you guys want it to be a sep. proj because you want to run it as a sep. build step. If I understand the initial issue and the blog post from @Aaronontheweb correctly you guys want to solve the issue of accidentally breaking the API. I consider the public API of a library an integral part of the library. Therefore when you change the lib (possibly introducing a breaking change) and you only run the tests you immediately get the feedback that you did break the API. Making it a sep. proj makes it again, an afterthought. The costs of running those ApprovalTests is low and the friction introduced by leaving them where they belong, in the corresponding specs/test lib of the corresponding project, is close to non-existant. Thoughts?

But hey, in the end, your project, your rules ;)

[Aaronontheweb]

I consider the public API of a library an integral part of the library. Therefore when you change the lib (possibly introducing a breaking change) and you only run the tests you immediately get the feedback that you did break the API. Making it a sep. proj makes it again, an afterthought. The costs of running those ApprovalTests is low and the friction introduced by leaving them where they belong, in the corresponding specs/test lib of the corresponding project, is close to non-existant. Thoughts?

This is just a technical thing @danielmarbach - we want to keep those API coverage tests in a separate project so those tests can be run independently on the build server separate from our massive test suite in that project. That's all - our CI process strives to keep things very granular and specific.

Edit: think of it this way - I want to see a red check mark specifically if backwards compat in the API breaks and have that called out by name in our build step list. Right now it's lumped into our "unit tests" bucket, which is used to indicate that a piece of functionality broke.

[danielmarbach]

Ok, I'll move it out. How about (of course in sep. PR)

• Persistence
• Others?

[Aaronontheweb]

@danielmarbach Akka.Persistence, Akka.Remote, Akka.Cluster, and Akka.DI.Core are the big ones

[danielmarbach]

@Aaronontheweb but in the same proj?

[Aaronontheweb]

btw, I just realized - you'll need to name the project

Akka.APIApproval.Tests
The F# script scans for the .Tests and uses that it build its list of assemblies that need to be tested

[Aaronontheweb]

@danielmarbach all in one project is fine

[danielmarbach]

How about Akka.API.Tests?

[Aaronontheweb]

sounds good 👍

[danielmarbach]

Here we go

[Aaronontheweb]

Build server is FUBAR at the moment - I'll need to circle back on this once I fix it.

[Aaronontheweb]

Looks good - just need to "approve" the current API, right @danielmarbach ?

[danielmarbach]

Hmmm... I did. is the build failing? Test was green on my machine

Am 05.01.2016 um 21:50 schrieb Aaron Stannard notifications@github.com:

Looks good - just need to "approve" the current API, right @danielmarbach ?

—
Reply to this email directly or view it on GitHub.

[Aaronontheweb]

Yes, the API approval is failing.

[Aaronontheweb]

@danielmarbach do you need to commit the approvals file?

[danielmarbach]

Yes, hmm... Did it on the first commit but not on the revamp apparently. i'll fix

Am 05.01.2016 um 22:47 schrieb Aaron Stannard notifications@github.com:

@danielmarbach do you need to commit the approvals file?

—
Reply to this email directly or view it on GitHub.

[danielmarbach]

Hold, on it's here

[danielmarbach]

@Aaronontheweb I think this is a classical line ending problem. Force pushed with the following .gitattribute changes:

*.txt text eol=crlf

Need to crash now. Hope that fixes the problem. If it doesn't you might need to diff on the agent.

[danielmarbach]

I did a

git rm --cached -r .
git reset --hard

on my machine, seems to be ok

[danielmarbach]

@Aaronontheweb can't really fix it since I don't have access to TC agents to verify. Can you check it? Does it run on your machine?

[Aaronontheweb]

@danielmarbach I'll take a look at this over the weekend - but yeah, looks like build server silliness is responsible

[Aaronontheweb]

@danielmarbach lol I think I figured out the issue - we may not have a mergetool that ApprovalTests can find installed on the build agents ;)

[Aaronontheweb]

Yep, fixed it - fired up a custom VM and tried to run the approval tests. Bailed out with a vague NullReferenceException. Appears to be that the issue was not having an appropriate diffing tool installed. Adding WinMerge helped. Re-imaging the VMs now.

[danielmarbach]

Thx! There is now way I could have figured that out without spelunking on the agents

Am 13.01.2016 um 01:51 schrieb Aaron Stannard notifications@github.com:

Yep, fixed it - fired up a custom VM and tried to run the approval tests. Bailed out with a vague NullReferenceException. Appears to be that the issue was not having an appropriate diffing tool installed. Adding WinMerge helped. Re-imaging the VMs now.

—
Reply to this email directly or view it on GitHub.

[Aaronontheweb]

@danielmarbach yep, absolutely right - the error message captured in the TeamCity log was different somehow. I also believe I've fixed #1622 with some TeamCity configuration changes. Running a bunch of different builds in parallel now to find out.

[danielmarbach]

Are you going to pull it in soon then?

Am 13.01.2016 um 06:25 schrieb Aaron Stannard notifications@github.com:

@danielmarbach yep, absolutely right - the error message captured in the TeamCity log was different somehow. I also believe I've fixed #1622 with some TeamCity configuration changes. Running a bunch of different builds in parallel now to find out.

—
Reply to this email directly or view it on GitHub.

[Aaronontheweb]

Closed via #1639

[Aaronontheweb]

@danielmarbach yep, all done! I picked up your commit and included it in #1639

[danielmarbach]

Any other API approvals that I should contribute?

Am 14.01.2016 um 04:06 schrieb Aaron Stannard notifications@github.com:

@danielmarbach yep, all done! I picked up your commit and included it in #1639

—
Reply to this email directly or view it on GitHub.