First draft of the CompleteApiScraper and ApiComparator (Issue #3239) #3253

iaronaraujo · 2018-02-07T19:20:50Z

Contains

This is the first draft of #3239 , but instead of creating a JUnit test to check API's differences, I created a new class with a main method (ApiComparator) since I wasn't sure what would be a "failure" or a "pass".

How to test

1 - Run the main method in the ApiSaver class and it will generate a file called "API_file.txt".
2 - Create a new public method on a class or interface / change an existing public method's arguments, return type or exception type.
3 - Run the main method in the ApiComparator class. It will generate a file called "New_API_file.txt" and compare both, printing a report with minor and major changes.

Outstanding before merging

This is the first draft to receive feedback, so there are still some spaces for refactoring
The report contains unnecessary things like prints from CompleteApiScraper and Classes' and Methods' names format is not good since it doesn't only print the name.
Changes to classes' constructors aren't being detected since I wasn't sure it was needed. Creation of new classes isn't detected either.

GooeyHub · 2018-02-07T19:34:11Z

Hooray Jenkins reported success with all tests good!

iaronaraujo · 2018-02-19T13:13:14Z

@oniatus Tagging you so you can take a look at this and see if it fits what you wanted.

Cervator

Nice work! While checking this out I put the before + after + console log snippet in a Gist to make it more easy to review without running it:

https://gist.github.com/Cervator/f34259ba73f7af7e02f3f2627211b143

I added a method hi to SectionDefinitionData as that came up first in my first scan. Did not stay in the same position for the second scan though.

I left some review comments, mostly the usual - javadoc, comments, code style. Make sure you have Checkstyle working in your IDE, I left most of those un-commented since the plugin should be able to tell you about every line affected :-)

As for changes:

Yep constructors definitely also need to be covered
As do new classes (by definition they'd require a minor version bump)
Could the empty parameters & exception lines be omitted? If not present just assume empty. Could also indent those lines one more - if present

Cervator · 2018-04-02T16:28:12Z

engine-tests/src/test/java/org/terasology/documentation/ApiScraper.java

@@ -84,6 +84,7 @@ public static void main(String[] args) throws Exception {
                    } else {
                        //System.out.println("Jar-based Class: " + apiClass + ", came from " + location);
                        sortedApi.put(category, apiClass.getName() + (apiClass.isInterface() ? " (INTERFACE)" : " (CLASS)"));
+


Stray newline? If only change in file reverting would make for a cleaner diff/history :-)

Cervator · 2018-04-02T16:28:26Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+import java.util.HashMap;
+
+/**
+ * Created by iaron on 04/02/2018.


Proper javadoc on all the things FTW! :-)

Cervator · 2018-04-02T16:29:12Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+import com.google.common.collect.Multimaps;
+import org.terasology.documentation.apiScraper.util.ApiMethod;
+
+import java.io.*;


Avoid star imports - make sure you set up Checkstyle for IDE highlight of errors, this file lights up like a 🎄 :-)

Cervator · 2018-04-02T16:29:39Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiSaver.java

+import java.io.FileWriter;
+
+
+public final class ApiSaver {


Moar javadoc please!

Cervator · 2018-04-02T16:30:13Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java

@@ -0,0 +1,135 @@
+/*
+ * Copyright 2015 MovingBlocks


Mildly old copy pasta :D Might want to bump all the headers to 2018

Cervator · 2018-04-02T16:30:51Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java

+    }
+
+    /**
+     *


Probably could still have a brief method description rather than solely rely on return + throws

Cervator · 2018-04-07T15:12:51Z

@iaronaraujo poke :-)

Also: just realized something. A new method is usually not a problem. Unless it is in an interface or abstract class forcing any existing implementors to implement it. That would be an API violation too.

How to detect that ... look for new methods in interfaces and abstract classes and if the line ends in a ; it is a violation?

iaronaraujo · 2018-04-07T15:33:36Z

Alright! A new method on an interface or abstract class should be reported as a minor or major change?

eviltak

Slight refactoring to conform to Terasology's code style.

eviltak · 2018-04-10T16:51:54Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java

+            }
+
+            if (location == null) {
+                System.out.println("Failed to get a class/package location, skipping " + apiClass);


This should be a warning/error logged via an class level org.slf4j.Logger constant created via LoggerFactory.getLogger(CompleteApiScraper.class). Same goes for all other System.out.print calls (except in the main methods used for testing).

eviltak · 2018-04-10T16:52:09Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java

+                    break;
+
+                default :
+                    System.out.println("Unknown protocol for: " + apiClass + ", came from " + location);


Same as above.

eviltak · 2018-04-10T16:54:51Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java

+
+                case "file" :
+                    // If file based we know it is local so organize it like that
+                    category = "terasology engine";


"terasology engine" should be replaced with a class-level constant. TERASOLOGY_API_CLASS_CATEGORY is a potential name, but you may choose an appropriate name on your own.

eviltak · 2018-04-10T16:56:27Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java

+                    System.out.println("Unknown protocol for: " + apiClass + ", came from " + location);
+            }
+        }
+        api.putAll("external", ExternalApiWhitelist.CLASSES.stream()


Consider extracting "external" to an appropriately named class-level constant.

eviltak · 2018-04-10T17:00:06Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/util/ApiMethod.java

+    }
+
+    public ApiMethod(){
+        this(DEFAULT_VALUE,DEFAULT_VALUE,DEFAULT_VALUE,DEFAULT_VALUE,DEFAULT_VALUE);


I am not sure if this constructor (or the default values) is required at all. The only uses that I can see are in the testing methods, and I am sure those can be worked around. Any specific reason why this constructor exists?

Yes, there is, but with some refactors I may be able to remove it

eviltak · 2018-04-10T17:08:43Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/util/ApiMethod.java

+        result = 31 * result + getName().hashCode();
+        result = 31 * result + getReturnType().hashCode();
+        result = 31 * result + getExceptionType().hashCode();
+        result = 31 * result + getParametersType().hashCode();


IntelliJ code generation is at it again! I suggest replacing the method body with a single call to Objects.hash.

…on the PR discussion. Added Javadoc and Comments

GooeyHub · 2018-04-12T18:36:50Z

Hooray Jenkins reported success with all tests good!

Cervator

Really solid and impressive work, thanks for all the updates :-) Checkstyle is happy and the doc is awesome. Added some comments with mostly minor tweaks for here and there. Haven't tested yet this time around due to lack of time but thought I'd get initial comments in

Cervator · 2018-04-13T03:49:28Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+
+
+/**
+ * Generates a "New_API_file.txt" and compares it with the "API_file.txt" to detect major and minor version increases.


General comment about Javadoc: the class-level comment should focus more on what the class is for, less so how it does that :-)

So in this case really the ApiComparator is meant to be used for better detecting API changes of various degrees. You could keep the file details (and especially filenames) closer to the methods that perform those functions

Cervator · 2018-04-13T03:52:30Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+
+/**
+ * Generates a "New_API_file.txt" and compares it with the "API_file.txt" to detect major and minor version increases.
+ * Major increases: Deletion of class, new abstract method, method deletion, existing method's change of


Slight more detail: also "new non-default interface method" yet not "deletion of internal methods such as a private or protected method, since that wouldn't be visible to an outside API user. Maybe just throw "public" on there, would also apply to changing parameters etc.

Cervator · 2018-04-13T03:55:30Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+ * Generates a "New_API_file.txt" and compares it with the "API_file.txt" to detect major and minor version increases.
+ * Major increases: Deletion of class, new abstract method, method deletion, existing method's change of
+ * parameters types, exception types or return type.
+ * Minor increases: Creation of a new class and new method in an interface or abstract class,


New method in an existing interface (that isn't a default method) should be a major change. New abstract method in an abstract class is also major, but new regular method in an abstract class is minor.

Interestingly, perhaps, the addition of a brand new interface (or abstract class) is indeed a minor change, not major (since nothing can be using it yet)

Also again "public" might be useful to throw in, since a new private method doesn't matter.

May also want to briefly note that any change not identified as major or minor would count as a patch-level change, just for completeness. Maybe link to https://semver.org/

Cervator · 2018-04-13T03:57:52Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+    }
+
+    /**
+     * Reads an api file and puts it's information in a map to be used in the api comparison


Trivial fix: its not it's

Could alternatively shorten to something like "Loads API info from file into a map to ..."

Cervator · 2018-04-13T03:59:12Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+     * Reads an api file and puts it's information in a map to be used in the api comparison
+     * @param br BufferedReader containing an api file content
+     * @return A map with the api classes and interfaces as keys;
+     * Their methods and as a list of ApiMethods in the values


I'm not sure how javadoc would interpret a newline in a @return description? I really haven't seen that before or considered it. Pretty much just single line descriptions, which this could probably be written as anyway :-)

Cervator · 2018-04-13T04:01:38Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+
+    /**
+     * Checks creation and deletion of methods, as well as existing method changes
+     * @param originalApi the original api generated from "API_file.txt"


You might want to move the "API_file.txt" and related to constants and refer to those instead

Cervator · 2018-04-13T04:02:34Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+                    System.out.println("=================================================================");
+                }
+            }
+


Another trivial note: you've got some odd extra newlines here and there :-)

Cervator · 2018-04-13T04:04:37Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/util/ApiMethod.java

+    private String parametersType;
+
+    /**
+     *


Another spot where you could use a main javadoc description (or else any actual generated javadoc would end up with some empty boxes here and there)

Cervator · 2018-04-13T04:20:19Z

Oh, also, one other request: please add the generated API files to the .gitignore :-)

GooeyHub · 2018-04-13T16:37:24Z

Hooray Jenkins reported success with all tests good!

Cervator

I merged this locally for final testing before pushing it, did a few tiny javadoc tweaks, but during testing found a couple expectations off:

a new public method in a non-abstract class is missed. This should be a minor change
a new default method in an interface is missed. Also minor change

There could be more but it is tricky to test since if you break API, well, in some cases the whole thing doesn't compile anymore so you can't test without first "fixing" a bunch of stuff :-)

I'm thinking we should consider two big things:

Write actual tests for the test - two sets of mock Java source that goes through every possible change type, with tests that go through and validate that yep, each is caught
Maybe pull most of this into its own utility repository. Especially if we start adding tests for a set of utility classes that begins getting kinda crazy. And this could be hugely useful elsewhere, say to do the same kind of checking for DestSol (I wonder if such a lib might already exist?)

A split out mini-lib could be depended on by Terasology with a requirement to implement the ApiScraper pieces, hopefully with some boilerplate code provided by the lib. Then the user (Terasology) gets to define what is actually part of the API (stuff marked with @API, on the whitelist, etc)

What do you think?

We could either try to go that route immediately or if you'd rather then finish up with the current expectation of SemVer, merge, then consider future improvements separately

For the additional Javadoc tweaks I pushed it for easier review in a commit than me leaving comments here, some are just adding . at the end of the first sentence of the main javadoc description. See Cervator@53bfeaa - ignore the commit message, I forgot to write a new one ;-)

Cervator · 2018-04-14T02:46:37Z

engine-tests/src/test/java/org/terasology/documentation/apiScraper/ApiComparator.java

+                        found = true;
+                    }
+                }
+                if (!found && isInterfaceOrAbstract(method2.getClassName())) {


This misses a new public method in a non-abstract class. That should count as a minor change :-)

I'm not sure if that was done previously and we're just going back and forth on what's included, or if it was legit missed

…thod is now being reported

GooeyHub · 2018-04-14T17:39:43Z

Hooray Jenkins reported success with all tests good!

iaronaraujo · 2018-04-14T17:42:21Z

I fixed the two points you mentioned.
I think it is a great idea to put this in its own repository because as you said it would make it way easier to create automatic tests for it and try to use it on DestSol. But I'd like to treat these changes as further improvements and have the current version merged because people could begin using it and giving me feedback, so when I put this in a new repository, I can improve it based on the feedback.

Cervator · 2018-04-15T03:49:05Z

Thanks a bunch! Merged :-)

Submitted three follow-up issues: #3320, #3321, #3322

First draft of the CompleteApiScraper and ApiComparator

d4b788a

Cervator requested changes Apr 2, 2018

View reviewed changes

eviltak suggested changes Apr 10, 2018

View reviewed changes

iaronaraujo added 2 commits April 12, 2018 11:53

Added increase cases and minor performance refactor

6264efa

Refactored code following CheckStyle's warnings and some suggestions …

c61ea40

…on the PR discussion. Added Javadoc and Comments

Cervator requested changes Apr 13, 2018

View reviewed changes

Refactored code's Javadoc and created constant for file names

8fca964

Cervator added a commit to Cervator/Terasology that referenced this pull request Apr 14, 2018

Merge PR MovingBlocks#3253 - API detection work

d3b76d3

Cervator requested changes Apr 14, 2018

View reviewed changes

Interface's new default method and a non-abstract class new public me…

dabd9e3

…thod is now being reported

Cervator merged commit dabd9e3 into MovingBlocks:develop Apr 15, 2018

Cervator added a commit that referenced this pull request Apr 15, 2018

Merge PR #3253 by @iaronaraujo - API scanning

dba360b

Cervator added this to the v2.0.0 milestone Apr 15, 2018

Cervator added the Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. label Apr 15, 2018

This was referenced Apr 15, 2018

API scanning improvement: hook up to PRs #3320

Open

API scanning improvement: write out a SemVer detection definition using unit tests #3321

Open

API scanning improvement: extract involved code to a new repo #3322

Open

This was referenced Apr 15, 2018

Create a demo PR how to extend ApiScraper for API Testing #3239

Closed

First attempt for api test #2919

Closed

Cervator mentioned this pull request Apr 26, 2018

API scanning improvement: Catagorize and attach javadoc in a generated report/doc site #3341

Open

Cervator mentioned this pull request Apr 30, 2020

Auto-doc block shapes for wiki with sample(s) included #508

Open

BenjaminAmos mentioned this pull request Jan 21, 2024

refactor: use logger instead of System.out.println #5188

Merged

+                  }
+                  /**
+                   *



		/**
		* Generates a "New_API_file.txt" and compares it with the "API_file.txt" to detect major and minor version increases.

First draft of the CompleteApiScraper and ApiComparator (Issue #3239) #3253

First draft of the CompleteApiScraper and ApiComparator (Issue #3239) #3253

Conversation

iaronaraujo commented Feb 7, 2018

Contains

How to test

Outstanding before merging

GooeyHub commented Feb 7, 2018

iaronaraujo commented Feb 19, 2018

Cervator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cervator commented Apr 7, 2018

iaronaraujo commented Apr 7, 2018

eviltak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eviltak Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooeyHub commented Apr 12, 2018

Cervator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cervator commented Apr 13, 2018

GooeyHub commented Apr 13, 2018

Cervator left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GooeyHub commented Apr 14, 2018

iaronaraujo commented Apr 14, 2018

Cervator commented Apr 15, 2018

eviltak Apr 10, 2018 •

edited

Loading

Cervator left a comment •

edited

Loading