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

Update API Approver reporting #4844

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

Arkatufus
Copy link
Contributor

@Arkatufus Arkatufus commented Mar 11, 2021

The reporting of Akka.API.Tests really needs an overhaul.

  1. It relies on a very flaky DiffEngine to try and guess the best diff viewer installed in the system based on the file extension.
  2. It assumes that the person running the test can access both the approved.txt and received.txt file to view the diff
  3. DiffEngine often fails, resulting in an exception being thrown.
  4. There is no diff viewer in the CI pipeline, making debugging impossible.

The better route is to actually embed the diff into the unit test report itself, so that it appears in the failing unit test report.
Diff message format:

<<<<<<<<< [ Approved file name ]
[ Line number ] + This line is newly added
[ Line number ] - This line is removed
[ Line number ] ? This line is modified
=========
[ Line number ] + This line is newly added
[ Line number ] - This line is removed
[ Line number ] ? This line is modified
>>>>>>>>> [ Received file name ]

@Arkatufus
Copy link
Contributor Author

Example failed output:

Akka.API.Tests.ApiNotApprovedException
Failed API approval. Diff:
<<<<<<<<< CoreAPISpec.ApproveCore.approved.txt
=========
[5060] +     public class static ArrayExtensions
[5061] +     {
[5062] +         public static bool IsNullOrEmpty(this System.Array obj) { }
[5063] +         public static bool NonEmpty(this System.Array obj) { }
[5064] +         public static void Shuffle<T>(this T[] array) { }
[5065] +         public static System.Collections.Generic.Dictionary<T, int> ZipWithIndex<T>(this System.Collections.Generic.IEnumerable<T> collection) { }
[5066] +     }
>>>>>>>>> CoreAPISpec.ApproveCore.received.txt
  Exception doesn't have a stacktrace

Copy link
Contributor

@IgorFedchenko IgorFedchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change anything that is documented here? https://getakka.net/community/public-api-changes.html

If it does not (documentation is up to date, just adds additional output ) - you can treat this PR as approved :)

@Arkatufus
Copy link
Contributor Author

Arkatufus commented Mar 12, 2021

Nothing is changed, it just adds an extra error reporting when the test catches a public API change, instead of a null referece exception

Copy link
Contributor

@IgorFedchenko IgorFedchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@IgorFedchenko IgorFedchenko merged commit 7fb3a5e into akkadotnet:dev Mar 12, 2021
@Arkatufus Arkatufus deleted the Update_ApiApprover branch April 22, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants