expand JSON test coverage; add datastore and cloud storage #443

elharo · 2017-08-25T12:38:13Z

@briandealwis @loosebazooka @garrettjonesgoogle @eamonnmcmanus @lesv

briandealwis · 2017-08-25T13:29:18Z

src/main/java/com/google/cloud/tools/libraries/MavenCoordinates.java

+
+  @Override 
+  public int hashCode() {
+    return (groupId + ":" + artifactId + ":" + version).hashCode();


Use Objects.hash()?

I prefer to avoid dependencies is possible. This seems simple enough and I like the connection with the Gradle dependency ID format.

briandealwis · 2017-08-25T13:29:53Z

src/main/java/com/google/cloud/tools/libraries/MavenCoordinates.java

+    if (o == null || !(o instanceof MavenCoordinates)) {
+      return false;
+    }
+    MavenCoordinates other = (MavenCoordinates) o;


equals() needs to check repository, classifier, and type. Use Object.equals()?

I did wonder about that. For this use case, I'm not sure I want those included. If we're pulling the same dependency from two repos, I want to flag that. Is it reasonable for us to have two different dependencies for the same artifact with different classifiers or types?

The classifier and type are pretty key coordinates. I think you should have a different method for identifying equivalence that ignores repository. Or perhaps the repository should not be a coordinate at all.

Now that you mention it, what I really need to verify here is that the groupId:artifactId is unique, not even including version.

briandealwis · 2017-08-25T13:33:19Z

src/test/java/com/google/cloud/tools/libraries/LibrariesTest.java

+      Assert.assertTrue(status, 
+          "beta".equals(status) || "alpha".equals(status) || "GA".equals(status));
+      new URI(client.getString("apireference"));
+      Assert.assertTrue("1.7.0".equals(client.getString("languageLevel")));


The precision seems odd.

It is. ATM, we don't have anything else though. I do want to make this a check for a real version string, not just a string.

briandealwis · 2017-08-25T13:40:45Z

src/test/java/com/google/cloud/tools/libraries/LibrariesTest.java

+    for (int i = 0; i < clients.size(); i++) {
+      JsonObject client = (JsonObject) clients.get(i);
+      String status = client.getString("status");
+      Assert.assertTrue(status, 


Could use Hamcrest's org.hamcrest.collection.IsArrayContaining so the invalid value is shown in the failure message.

briandealwis · 2017-08-25T14:03:56Z

src/test/java/com/google/cloud/tools/libraries/MavenCoordinatesTest.java

+
+import org.junit.Test;
+
+public class MavenCoordinatesTest {


Need some tests for equals()

loosebazooka · 2017-08-25T15:43:46Z

The PR says it's expanding JSON test coverages but makes changes to a non-test class? Is there more going on here?

elharo · 2017-08-25T17:15:02Z

We needed MavenCoordinates for the tests, or did at one point. Now I'm not so sure. I expect there'll be some libraries for processing the metadata, though I'm still not sure which piece swill land here and which in the IDE repos.

loosebazooka · 2017-08-25T18:02:41Z

If it's only being used for tests, can it be in the src/test/java directory instead?

elharo · 2017-08-25T18:30:44Z

reverted MavenCoordinates now; it can come back later if needed

loosebazooka · 2017-08-25T18:34:19Z

src/test/java/com/google/cloud/tools/libraries/LibrariesTest.java

+
+  private static final String[] statuses = {"alpha", "beta", "GA"};
+
+  private static void verifyApi(JsonObject api) throws URISyntaxException {


nit : would using assertApi instead of verifyApi clarify that the intention of the method is to make assertions?

loosebazooka · 2017-08-25T18:53:01Z

src/test/java/com/google/cloud/tools/libraries/LibrariesTest.java

-      new URI(api.getString("documentation"));
-      if (api.getString("icon") != null) {
-        new URI(api.getString("icon"));
+    Map<String, String> map = new HashMap<>();


name of map maybe can be more descriptive.

loosebazooka · 2017-08-25T18:54:11Z

src/test/java/com/google/cloud/tools/libraries/LibrariesTest.java

+  }
+
+  @Test
+  public void testMavenCoordinates() throws FileNotFoundException, URISyntaxException {


could this be named testMavenCoordinate_fileIsValid or something? It's hard for me to tell (since I didn't really look at the previous CL) what this test is doing.

codecov · 2017-08-25T19:56:45Z

Codecov Report

Merging #443 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #443   +/-   ##
=======================================
  Coverage   66.28%   66.28%           
=======================================
  Files          67       67           
  Lines        1815     1815           
  Branches      281      281           
=======================================
  Hits         1203     1203           
  Misses        489      489           
  Partials      123      123

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a6ec35c...05e0d09. Read the comment docs.

briandealwis · 2017-08-28T15:20:16Z

src/main/java/com/google/cloud/tools/libraries/libraries.json

      "language":"java",
      "site":"https://cloud.google.com/bigquery/docs/reference/libraries#client-libraries-install-java",
-      "apireference":"https://googlecloudplatform.github.io/google-cloud-java/0.22.0/apidocs/com/google/cloud/bigquery/package-summary.html",
-      "version" : "0.22.0",
+      "apireference":"https://googlecloudplatform.github.io/google-cloud-java/latest/apidocs/com/google/cloud/bigquery/package-summary.html",


Would this be clearer as a language-specific field called javadoc?
Oh, actually we should have a javadoc field for programmatic access (e.g., Eclipse javadoc hovers) as well as a the more human-meaningful API starting point.

what would the difference be. For example, what javadoc URL would Eclipse need here?

When I try setting the Javadoc URL for a jar to this URL (right-click on a jar, like a bundle in the Plug-in Dependencies, and select Javadoc Location, and set as a URL), Eclipse shown an error:

The java docs have a specific layout, as does the HTML. So Eclipse is able to find and parse out the appropriate Javadoc to be shown in a hover.

To make that work, I think we first need to get google-cloud-java to publish individual javadoc for each API. The file it's looking for is at

https://googlecloudplatform.github.io/google-cloud-java/latest/apidocs/

We should probably pull IDE readable source and javadoc jars from Maven Central rather than calling them out as a separate artifact.

+1 to @elharo 's comment - then the javadoc and source will actually match the classes that the user is using.

CT for IntelliJ plans to expose these links to the end user so it needs human browsable URLs.

The links to the Maven source/javadoc jars don't need to be explicitly included here because they can be derived from the Maven coordinates.

Sounds good.

briandealwis · 2017-08-28T15:21:55Z

src/main/java/com/google/cloud/tools/libraries/libraries.json

      "language":"java",
      "site":"https://cloud.google.com/bigquery/docs/reference/libraries#client-libraries-install-java",
-      "apireference":"https://googlecloudplatform.github.io/google-cloud-java/0.22.0/apidocs/com/google/cloud/bigquery/package-summary.html",
-      "version" : "0.22.0",
+      "apireference":"https://googlecloudplatform.github.io/google-cloud-java/latest/apidocs/com/google/cloud/bigquery/package-summary.html",
      "infotip":"The com.google.cloud.bigquery packages",


This might be useful to encode in the metadata to enable UI discovery (e.g., a quick-fix on unresolved symbols)

can you elaborate? I'm not sure exactly what you're proposing here.

Expressing this as:

"javaPackages":["com.google.cloud.bigquery", "com.google.cloud.bigquery.**"],

For example, PDE contributes a quick-fix to suggesting adding an Import-Package or Require-Bundle on an unresolved symbol that is found in a bundle:

That would be something different. This is human readable text for the infotip on the library.

It would be good to have this in a machine readable format too for the tooling reasons above.

Maybe it would help to add additional information here. For example, there are sometimes alpha versions of libraries that users may want to try. Or if this is the Google API Client library, we could add a pointer to the Google Client library?

Sounds good. How do we tell Eclipse about our packages when adding a library?

I think we'd have a quick-fix on an unresolved symbol like PDE's quick-fix. Our quick-fix would iterate on the library definitions to see if it can find a match.

oops, ping on this request too.

briandealwis · 2017-08-28T15:24:58Z

src/main/java/com/google/cloud/tools/libraries/libraries.json

+    "name": "Google Cloud Datastore",
+    "documentation": "https://cloud.google.com/datastore/docs/reference/libraries",
+    "description":"Google Cloud Datastore is a NoSQL key-value database built for automatic scaling.",
+    "transport": "http",


Should transport be an array? What do we do for libraries that support both http and grpc?

Shouldn't this really be in the clients section? We may have libraries that just use the http REST APIs, even though the API supports both

do any libraries support more than one transport? let me check with the api team.

still waiting for a reply but the docs do say, "Clients in this repository use either HTTP or gRPC for the transport layer. The README of each client documents the transport layer the client uses."

Stepping back, what will we do with this information? Are we going to use it as a filter — e.g., if grpc then cannot be used on AESv7?

Yes, exactly that.

So is better to encode this platform-compatibility knowledge into the consumers of this API, or to make it explicit in a compatibility/platform section? I lean towards the latter.

That brings us back to where we were earlier with defining in this file all the possible platforms, and it all resolves to the transport anyway. For now, I'm inclined not to encode platforms. We can add those later if we need them.

I have turned this into an array though.

briandealwis · 2017-08-28T15:32:40Z

src/main/java/com/google/cloud/tools/libraries/libraries.json

+      {
+      "name":"Google Cloud Storage Client Library for Java",
+      "language":"java",
+      "site":"https://cloud.google.com/storage/docs/reference/libraries#client-libraries-install-java",


Wouldn't the #using_the_client_library section be more appropriate, since the user will be using the consuming UI to automate the installation of this library?

What I really wanted was the entire page, but that won't pick up Java as opposed to Python. This is the nearest link to the top.

Let me file a bug requesting a URL for the whole page.

briandealwis · 2017-08-28T15:36:15Z

src/test/java/com/google/cloud/tools/libraries/LibrariesTest.java

+          ((JsonObject) api.getJsonArray("clients").get(0)).getJsonObject("mavenCoordinates");
+      String mavenCoordinates = 
+          coordinates.getString("groupId") + ":" + coordinates.getString("artifactId");
+      if (apiCoordinates.containsValue(mavenCoordinates)) {


It should be ok to provide different versions of an API library, shouldn't it? E.g., Java libraries for BigQuery 1.0.0 vs 2.0.0?

I think that would have a different group or artifact ID. I don't think we want both 1.3.0 and 1.4.0.

garrettjonesgoogle · 2017-08-30T14:22:32Z

src/main/java/com/google/cloud/tools/libraries/libraries.json

+      "site":"https://cloud.google.com/datastore/docs/reference/libraries#client-libraries-install-java",
+      "apireference":"https://googlecloudplatform.github.io/google-cloud-java/latest/apidocs/index.html?com/google/cloud/datastore/package-summary.html",
+      "infotip":"The com.google.cloud.datastore packages",
+      "status" : "GA",  


I think releaseLevel would be a more accurate description of the Alpha/Beta/GA values

wikipedia seems to call this a "stage" though I can't say I've heard that before: https://en.wikipedia.org/wiki/Software_release_life_cycle#Stages_of_development

It looks like "launch stage" is actually the terminology used by Google Cloud Platform: https://cloud.google.com/terms/launch-stages

However, we have to keep in mind that the release level of client libraries will lag behind the service.

We could offer separate statuses for the API and the client library. This one is specific to the client library. Meanwhile I'll change this to launchStage.

garrettjonesgoogle · 2017-08-30T14:23:10Z

src/main/java/com/google/cloud/tools/libraries/libraries.json

+      "mavenCoordinates" : {
+         "groupId" : "com.google.cloud",
+         "artifactId" : "google-cloud-datastore",
+         "version" : "1.4.0"


Will this version have to be updated with every google-cloud-java release?

Of course clients do have the option to ignore the version here and query Maven central for the latest release version. CT4E contains code to do this.

briandealwis · 2017-08-31T18:11:22Z

src/main/java/com/google/cloud/tools/libraries/libraries.json

      "language":"java",
      "site":"https://cloud.google.com/bigquery/docs/reference/libraries#client-libraries-install-java",
-      "apireference":"https://googlecloudplatform.github.io/google-cloud-java/0.22.0/apidocs/com/google/cloud/bigquery/package-summary.html",
-      "version" : "0.22.0",
+      "apireference":"https://googlecloudplatform.github.io/google-cloud-java/latest/apidocs/com/google/cloud/bigquery/package-summary.html",


Sounds good.

briandealwis · 2017-08-31T18:13:09Z

src/main/java/com/google/cloud/tools/libraries/libraries.json

+    "name": "Google Cloud Storage",
+    "documentation": "https://cloud.google.com/storage/docs/reference/libraries",
+    "description":"Unified BLOB storage for developers and enterprises, from live data serving to data analytics/ML to data archiving.",
+    "transport": ["http"],


Pluralize transports? since we have clients and mavenCoordinates?

elharo · 2017-09-05T10:02:11Z

Pinging everyone. Are there any final blockers that prevent us from checking this in and moving forward with the rest of the google-cloud-java APIs?

We can easily add additional content later. Changing or removing existing fields should be done here if possible.

If not, it needs an approval.

elharo

Ping ping ping

loosebazooka

This might also be a good time to begin the process of separating out our plugin support libraries.

briandealwis

LGTM too.

* expand JSON test coverage * add test for equals * hamcrest * redundant assert removed * fix loop * fix loop * regex versions * don't make repo a corrdinate * type to packaging * verify group ID artifact ID * checkstyle fixes * revert metadata for now * code review * add datastore api * toArray * GCS * make transport an array * make transport an array * launchStage * pluralize transports * pluralize transports * add packages

expand JSON test coverage

833b073

elharo requested a review from briandealwis August 25, 2017 12:38

googlebot added the cla: yes label Aug 25, 2017

briandealwis reviewed Aug 25, 2017

View reviewed changes

add test for equals

41e8f29

Elliotte Harold added 8 commits August 25, 2017 11:46

hamcrest

896ac8f

redundant assert removed

500ee93

fix loop

eb8283a

fix loop

568fd0e

regex versions

653117f

don't make repo a corrdinate

4de1ae1

type to packaging

6146737

verify group ID artifact ID

4e6949f

Elliotte Harold added 2 commits August 25, 2017 14:21

checkstyle fixes

26e02d9

revert metadata for now

a29e993

loosebazooka reviewed Aug 25, 2017

View reviewed changes

Elliotte Harold added 3 commits August 25, 2017 15:04

code review

ec13151

add datastore api

7ec06f6

toArray

6e23e11

GCS

fd5b606

elharo changed the title ~~expand JSON test coverage~~ expand JSON test coverage; add datastore and BigQuery Aug 28, 2017

elharo changed the title ~~expand JSON test coverage; add datastore and BigQuery~~ expand JSON test coverage; add datastore and cloud storage Aug 28, 2017

briandealwis reviewed Aug 28, 2017

View reviewed changes

garrettjonesgoogle reviewed Aug 30, 2017

View reviewed changes

Elliotte Harold added 3 commits August 30, 2017 14:02

make transport an array

867bd07

make transport an array

abaf735

launchStage

7517ae3

briandealwis reviewed Aug 31, 2017

View reviewed changes

Elliotte Harold added 3 commits August 31, 2017 16:43

pluralize transports

88a622e

pluralize transports

2c8e5d8

add packages

05e0d09

elharo commented Sep 8, 2017

View reviewed changes

loosebazooka approved these changes Sep 8, 2017

View reviewed changes

briandealwis approved these changes Sep 8, 2017

View reviewed changes

elharo merged commit 8193135 into master Sep 8, 2017

elharo deleted the metadata branch September 8, 2017 16:53

JoeWang1127 pushed a commit that referenced this pull request Nov 29, 2023

Remove codecov (#443)

a9202ef

JoeWang1127 pushed a commit that referenced this pull request Nov 29, 2023

docs: link to relevant configuration sections (#443)

5d39c93

release-please bot mentioned this pull request Dec 13, 2023

chore(main): release 0.11.0 #974

Closed

release-please bot mentioned this pull request Jan 17, 2024

chore(main): release 2.6.0 #980

Merged


		private static final String[] statuses = {"alpha", "beta", "GA"};

		private static void verifyApi(JsonObject api) throws URISyntaxException {

expand JSON test coverage; add datastore and cloud storage #443

expand JSON test coverage; add datastore and cloud storage #443

Conversation

elharo commented Aug 25, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loosebazooka commented Aug 25, 2017 • edited

elharo commented Aug 25, 2017

loosebazooka commented Aug 25, 2017

elharo commented Aug 25, 2017 • edited

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

codecov bot commented Aug 25, 2017 • edited

Codecov Report

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

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

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

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

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

elharo commented Sep 5, 2017

elharo left a comment

Choose a reason for hiding this comment

loosebazooka left a comment • edited

Choose a reason for hiding this comment

briandealwis left a comment

Choose a reason for hiding this comment

elharo commented Aug 25, 2017 •

edited

loosebazooka commented Aug 25, 2017 •

edited

elharo commented Aug 25, 2017 •

edited

codecov bot commented Aug 25, 2017 •

edited

loosebazooka left a comment •

edited