zentol commented on a change in pull request #14826:
URL: https://github.com/apache/flink/pull/14826#discussion_r571733119
##########
File path:
flink-docs/src/test/java/org/apache/flink/docs/configuration/ConfigOptionsDocsCompletenessITCase.java
##########
@@ -222,18 +222,8 @@ private static void compareDocumentedAndExistingOptions(
}
}
- private static Map<String, List<DocumentedOption>>
parseDocumentedCommonOptions()
Review comment:
let's move this removal into a separate hotfix commit
##########
File path:
flink-runtime-web/src/test/java/org/apache/flink/runtime/rest/compatibility/CompatibilityRoutines.java
##########
@@ -173,6 +194,13 @@ private static void assertCompatible(
"Previously optional query parameter %s is
now mandatory.",
oldParam.key));
}
+
+ if (!newParam.description.equals(oldParam.description)) {
Review comment:
these routines are not used by the completeness test, are they? Why do
we have to extend them?
##########
File path:
flink-docs/src/main/java/org/apache/flink/docs/rest/RestAPIDocGenerator.java
##########
@@ -334,6 +375,32 @@ private static JsonSchema generateSchema(Class<?>
messageClass) {
}
}
+ /** Query parameters for rest API. */
+ @VisibleForTesting
+ public static final class QueryParameter {
+ public String key;
+ public boolean isMandatory;
+ public String description;
+
+ QueryParameter(String key, boolean isMandatory, String description) {
+ this.key = key;
+ this.isMandatory = isMandatory;
+ this.description = description;
+ }
+ }
+
+ /** Path parameters for rest API. */
+ @VisibleForTesting
+ public static final class PathParameter {
Review comment:
seems odd that the parser code is not in the test scope.
##########
File path:
flink-docs/src/main/java/org/apache/flink/docs/rest/RestAPIDocGenerator.java
##########
@@ -281,11 +300,33 @@ private static String createQueryParameterHtmlList(
String.format(
"<li><code>%s</code> (%s):
%s</li>\n",
parameter.getKey(),
- parameter.isMandatory() ?
"mandatory" : "optional",
+ parameter.isMandatory()
+ ? MANDATORY_TAG
+ : "optional",
parameter.getDescription())));
return queryParameterList.toString();
}
+ @VisibleForTesting
+ public static PathParameter unformatPathParameterViaLiTagText(String text)
{
Review comment:
let's an example text in here the code easier to reason about
##########
File path: flink-docs/src/test/java/org/apache/flink/docs/TestUtils.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.docs;
+
+import java.io.File;
+
+/** Utils for tests of doc options. */
+public class TestUtils {
+ public static String getProjectRootDir() {
Review comment:
Let's have this return a `Path` now that it is shared between classes;
should make it easier to use.
##########
File path:
flink-docs/src/main/java/org/apache/flink/docs/rest/RestAPIDocGenerator.java
##########
@@ -281,11 +300,33 @@ private static String createQueryParameterHtmlList(
String.format(
"<li><code>%s</code> (%s):
%s</li>\n",
parameter.getKey(),
- parameter.isMandatory() ?
"mandatory" : "optional",
+ parameter.isMandatory()
+ ? MANDATORY_TAG
+ : "optional",
parameter.getDescription())));
return queryParameterList.toString();
}
+ @VisibleForTesting
+ public static PathParameter unformatPathParameterViaLiTagText(String text)
{
+ int keyEnd = text.indexOf(" - ");
+ Preconditions.checkArgument(keyEnd > 0);
+ String key = text.substring(0, keyEnd);
+ String description = text.substring(keyEnd + 3);
+ return new PathParameter(key, description);
+ }
+
+ @VisibleForTesting
+ public static QueryParameter unformatQueryParameterViaLiTagText(String
text) {
+ int keyEnd = text.indexOf(" (");
Review comment:
Add an example text as noted above
##########
File path:
flink-docs/src/main/java/org/apache/flink/docs/rest/RestAPIDocGenerator.java
##########
@@ -281,11 +300,33 @@ private static String createQueryParameterHtmlList(
String.format(
"<li><code>%s</code> (%s):
%s</li>\n",
parameter.getKey(),
- parameter.isMandatory() ?
"mandatory" : "optional",
+ parameter.isMandatory()
+ ? MANDATORY_TAG
+ : "optional",
parameter.getDescription())));
return queryParameterList.toString();
}
+ @VisibleForTesting
+ public static PathParameter unformatPathParameterViaLiTagText(String text)
{
+ int keyEnd = text.indexOf(" - ");
+ Preconditions.checkArgument(keyEnd > 0);
+ String key = text.substring(0, keyEnd);
+ String description = text.substring(keyEnd + 3);
+ return new PathParameter(key, description);
+ }
+
+ @VisibleForTesting
+ public static QueryParameter unformatQueryParameterViaLiTagText(String
text) {
Review comment:
could these be package-private?
##########
File path:
flink-docs/src/test/java/org/apache/flink/docs/rest/RestOptionsDocsCompletenessITCase.java
##########
@@ -0,0 +1,281 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.docs.rest;
+
+import org.apache.flink.api.java.tuple.Tuple2;
+import org.apache.flink.docs.TestUtils;
+import org.apache.flink.runtime.rest.compatibility.CompatibilityRoutines;
+import org.apache.flink.runtime.rest.versioning.RestAPIVersion;
+
+import
org.apache.flink.shaded.jackson2.com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import
org.apache.flink.shaded.jackson2.com.fasterxml.jackson.annotation.JsonProperty;
+import
org.apache.flink.shaded.jackson2.com.fasterxml.jackson.databind.ObjectMapper;
+
+import org.jsoup.Jsoup;
+import org.jsoup.nodes.Document;
+import org.jsoup.nodes.Element;
+import org.jsoup.select.Elements;
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+/** This test verifies that all rest docs are documented well with latest rest
APIs. */
+@RunWith(Parameterized.class)
+public class RestOptionsDocsCompletenessITCase {
+ private static final String SNAPSHOT_RESOURCE_PATTERN =
"rest_api_%s.snapshot";
+
+ @Parameterized.Parameters(name = "version = {0}")
+ public static Iterable<RestAPIVersion> getStableVersions() {
+ return Arrays.stream(RestAPIVersion.values())
+ .filter(RestAPIVersion::isStableVersion)
+ .collect(Collectors.toList());
+ }
+
+ private final RestAPIVersion apiVersion;
+
+ private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
+
+ public RestOptionsDocsCompletenessITCase(RestAPIVersion apiVersion) {
+ this.apiVersion = apiVersion;
+ }
+
+ @Test
+ public void testCompleteness() throws IOException {
+ final String versionedSnapshotFileName =
+ String.format(SNAPSHOT_RESOURCE_PATTERN,
apiVersion.getURLVersionPrefix());
+
+ final String rootDir = TestUtils.getProjectRootDir();
+
+ RestAPISnapshot restAPISnapshot = parseRestSnapshot(rootDir,
versionedSnapshotFileName);
+
+ Map<String, DocumentedOption> documentedOptions =
+ parseDocumentedOptions(rootDir,
apiVersion.getURLVersionPrefix());
+
+ for (RestAPI restAPI : restAPISnapshot.calls) {
+ // we will never check undocumented option.
+ if (RestAPIDocGenerator.UNDOCUMENTED_APIS.contains(restAPI.url)) {
+ continue;
+ }
+ DocumentedOption documentedOption =
+ documentedOptions.remove(getUniqueKey(restAPI.url,
restAPI.method));
+ Assert.assertNotNull(documentedOption);
+ Assert.assertEquals(restAPI.method, documentedOption.method);
+ Assert.assertEquals(restAPI.statusCode,
documentedOption.statusCode);
+ Assert.assertEquals(restAPI.methodDescription,
documentedOption.methodDescription);
+
+ Map<String, String> expectedPathParameters =
+ restAPI.pathParameters.pathParameters.stream()
+ .collect(Collectors.toMap(t -> t.key, t ->
t.description));
+ Map<String, String> actualPathParameters =
+ documentedOption.pathParameters.stream()
+ .collect(Collectors.toMap(t -> t.key, t ->
t.description));
+ Assert.assertEquals(expectedPathParameters, actualPathParameters);
+
+ Map<String, Tuple2<Boolean, String>> expectedQueryParameters =
+ restAPI.queryParameters.queryParameters.stream()
+ .collect(
+ Collectors.toMap(
+ t -> t.key,
+ t -> Tuple2.of(t.mandatory,
t.description)));
+ Map<String, Tuple2<Boolean, String>> actualQueryParameters =
+ documentedOption.queryParameters.stream()
+ .collect(
+ Collectors.toMap(
+ t -> t.key,
+ t -> Tuple2.of(t.isMandatory,
t.description)));
+ Assert.assertEquals(expectedQueryParameters,
actualQueryParameters);
+ }
+
+ Assert.assertTrue(documentedOptions.isEmpty());
+ }
+
+ private static final class DocumentedOption {
+ private final String url;
+ private final String method;
+ private final String statusCode;
+ private final String methodDescription;
+ private final Collection<RestAPIDocGenerator.PathParameter>
pathParameters;
+ private final Collection<RestAPIDocGenerator.QueryParameter>
queryParameters;
+
+ private DocumentedOption(
+ String url,
+ String method,
+ String statusCode,
+ String methodDescription,
+ Collection<RestAPIDocGenerator.PathParameter> pathParameters,
+ Collection<RestAPIDocGenerator.QueryParameter>
queryParameters) {
+ this.url = url;
+ this.method = method;
+ this.statusCode = statusCode;
+ this.methodDescription = methodDescription;
+ this.pathParameters = pathParameters;
+ this.queryParameters = queryParameters;
+ }
+ }
+
+ private static RestAPISnapshot parseRestSnapshot(
+ String rootDir, String versionedSnapshotFileName) throws
IOException {
+ final Path restSnapshotFolder =
+ Paths.get(rootDir, "flink-runtime-web", "src", "test",
"resources")
Review comment:
since the flink-runtime-web test-jar is on the classpath you could
instead load this file as a resource instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
