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

Integ Test Refactoring #1383

Merged
Merged
8 changes: 4 additions & 4 deletions .github/workflows/sql-test-and-build-workflow.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,11 @@ jobs:
matrix:
entry:
- { os: ubuntu-latest, java: 11 }
- { os: windows-latest, java: 11, os_build_args: -x doctest -x integTest -x jacocoTestReport -PbuildPlatform=windows }
- { os: macos-latest, java: 11, os_build_args: -x doctest -x integTest -x jacocoTestReport }
- { os: windows-latest, java: 11, os_build_args: -x doctest -PbuildPlatform=windows }
- { os: macos-latest, java: 11}
- { os: ubuntu-latest, java: 17 }
- { os: windows-latest, java: 17, os_build_args: -x doctest -x integTest -x jacocoTestReport -PbuildPlatform=windows }
- { os: macos-latest, java: 17, os_build_args: -x doctest -x integTest -x jacocoTestReport }
- { os: windows-latest, java: 17, os_build_args: -x doctest -PbuildPlatform=windows }
- { os: macos-latest, java: 17 }
runs-on: ${{ matrix.entry.os }}

steps:
Expand Down
8 changes: 7 additions & 1 deletion build-tools/sqlplugin-coverage.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* cluster is stopped and dump it to a file. Luckily our current security policy seems to allow this. This will also probably
* break if there are multiple nodes in the integTestCluster. But for now... it sorta works.
*/
import org.apache.tools.ant.taskdefs.condition.Os
apply plugin: 'jacoco'

// Get gradle to generate the required jvm agent arg for us using a dummy tasks of type Test. Unfortunately Elastic's
Expand Down Expand Up @@ -45,7 +46,12 @@ integTest.runner {
}

testClusters.integTest {
jvmArgs " ${dummyIntegTest.jacoco.getAsJvmArg()}"
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
// Replacing build with absolute path to fix the error "error opening zip file or JAR manifest missing : /build/tmp/expandedArchives/..../jacocoagent.jar"
jvmArgs " ${dummyIntegTest.jacoco.getAsJvmArg()}".replace('build',"${buildDir}")
} else {
jvmArgs " ${dummyIntegTest.jacoco.getAsJvmArg()}".replace('javaagent:','javaagent:/')
}
systemProperty 'com.sun.management.jmxremote', "true"
systemProperty 'com.sun.management.jmxremote.authenticate', "false"
systemProperty 'com.sun.management.jmxremote.port', "7777"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.wiredforcode.gradle.spawn

import org.apache.tools.ant.taskdefs.condition.Os
import org.gradle.api.tasks.TaskAction

class KillProcessTask extends DefaultSpawnTask {
Expand All @@ -12,7 +13,13 @@ class KillProcessTask extends DefaultSpawnTask {
}

def pid = pidFile.text
def process = "kill $pid".execute()
def killCommandLine
if (Os.isFamily(Os.FAMILY_WINDOWS)) {
killCommandLine = Arrays.asList("taskkill", "/F", "/T", "/PID", "$pid")
} else {
killCommandLine = Arrays.asList("kill", "$pid")
}
def process = killCommandLine.execute()

try {
process.waitFor()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,18 @@ class SpawnProcessTask extends DefaultSpawnTask {
}

private int extractPidFromProcess(Process process) {
def pidField = process.class.getDeclaredField('pid')
pidField.accessible = true

return pidField.getInt(process)
def pid
try {
// works since java 9
def pidMethod = process.class.getDeclaredMethod('pid')
pidMethod.setAccessible(true)
pid = pidMethod.invoke(process)
} catch (ignored) {
// fallback to UNIX-only implementation
def pidField = process.class.getDeclaredField('pid')
pidField.accessible = true
pid = pidField.getInt(process)
}
return pid
}
}
16 changes: 11 additions & 5 deletions doctest/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,12 @@ task startPrometheus(type: SpawnProcessTask) {

//evaluationDependsOn(':')
task startOpenSearch(type: SpawnProcessTask) {
command "${path}/gradlew -p ${plugin_path} runRestTestCluster"
if( getOSFamilyType() == "windows") {
command "${path}\\gradlew.bat -p ${plugin_path} runRestTestCluster"
}
else {
command "${path}/gradlew -p ${plugin_path} runRestTestCluster"
}
ready 'started'
}

Expand Down Expand Up @@ -94,12 +99,13 @@ task stopPrometheus() {
}
}
}

stopPrometheus.mustRunAfter startPrometheus
if(getOSFamilyType() != "windows") {
stopPrometheus.mustRunAfter startPrometheus
startOpenSearch.dependsOn startPrometheus
stopOpenSearch.finalizedBy stopPrometheus
}
doctest.dependsOn startOpenSearch
startOpenSearch.dependsOn startPrometheus
doctest.finalizedBy stopOpenSearch
stopOpenSearch.finalizedBy stopPrometheus
check.dependsOn doctest
clean.dependsOn(cleanBootstrap)

Expand Down
14 changes: 10 additions & 4 deletions integ-test/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
* under the License.
*/

import org.gradle.nativeplatform.platform.internal.DefaultNativePlatform
import org.opensearch.gradle.test.RestIntegTestTask
import org.opensearch.gradle.testclusters.StandaloneRestIntegTestTask

Expand Down Expand Up @@ -153,9 +152,10 @@ stopPrometheus.mustRunAfter startPrometheus
// Run PPL ITs and new, legacy and comparison SQL ITs with new SQL engine enabled
integTest {
dependsOn ':opensearch-sql-plugin:bundlePlugin'
dependsOn startPrometheus
finalizedBy stopPrometheus

if(getOSFamilyType() != "windows") {
dependsOn startPrometheus
finalizedBy stopPrometheus
}
systemProperty 'tests.security.manager', 'false'
systemProperty('project.root', project.projectDir.absolutePath)

Expand All @@ -180,6 +180,12 @@ integTest {
}
}

if(getOSFamilyType() == "windows") {
exclude 'org/opensearch/sql/ppl/PrometheusDataSourceCommandsIT.class'
exclude 'org/opensearch/sql/ppl/ShowDataSourcesCommandIT.class'
exclude 'org/opensearch/sql/ppl/InformationSchemaCommandIT.class'
}

exclude 'org/opensearch/sql/doctest/**/*IT.class'
exclude 'org/opensearch/sql/correctness/**'

Expand Down
24 changes: 12 additions & 12 deletions integ-test/src/test/java/org/opensearch/sql/ppl/CsvFormatIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ public void sanitizeTest() throws IOException {
String result = executeCsvQuery(
String.format(Locale.ROOT, "source=%s | fields firstname, lastname", TEST_INDEX_BANK_CSV_SANITIZE));
assertEquals(
"firstname,lastname\n"
+ "'+Amber JOHnny,Duke Willmington+\n"
+ "'-Hattie,Bond-\n"
+ "'=Nanette,Bates=\n"
+ "'@Dale,Adams@\n"
+ "\",Elinor\",\"Ratliff,,,\"\n",
"firstname,lastname%n"
+ "'+Amber JOHnny,Duke Willmington+%n"
+ "'-Hattie,Bond-%n"
+ "'=Nanette,Bates=%n"
+ "'@Dale,Adams@%n"
+ "\",Elinor\",\"Ratliff,,,\"%n",
result);
}

Expand All @@ -38,12 +38,12 @@ public void escapeSanitizeTest() throws IOException {
String result = executeCsvQuery(
String.format(Locale.ROOT, "source=%s | fields firstname, lastname", TEST_INDEX_BANK_CSV_SANITIZE), false);
assertEquals(
"firstname,lastname\n"
+ "+Amber JOHnny,Duke Willmington+\n"
+ "-Hattie,Bond-\n"
+ "=Nanette,Bates=\n"
+ "@Dale,Adams@\n"
+ "\",Elinor\",\"Ratliff,,,\"\n",
"firstname,lastname%n"
+ "+Amber JOHnny,Duke Willmington+%n"
+ "-Hattie,Bond-%n"
+ "=Nanette,Bates=%n"
+ "@Dale,Adams@%n"
+ "\",Elinor\",\"Ratliff,,,\"%n",
result);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,17 @@

package org.opensearch.sql.ppl;

import org.json.JSONObject;
import org.junit.jupiter.api.Test;
import org.opensearch.client.Request;
import org.opensearch.client.ResponseException;

import java.io.IOException;

import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_DOG;
import static org.opensearch.sql.util.MatcherUtils.columnName;
import static org.opensearch.sql.util.MatcherUtils.rows;
import static org.opensearch.sql.util.MatcherUtils.verifyColumn;
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;

import java.io.IOException;
import org.json.JSONObject;
import org.junit.jupiter.api.Test;
import org.opensearch.client.Request;
import org.opensearch.client.ResponseException;

public class DescribeCommandIT extends PPLIntegTestCase {

@Override
Expand Down Expand Up @@ -88,25 +86,4 @@ public void describeCommandWithoutIndexShouldFailToParse() throws IOException {
assertTrue(e.getMessage().contains("Failed to parse query due to offending symbol"));
}
}

@Test
public void testDescribeCommandWithPrometheusCatalog() throws IOException {
JSONObject result = executeQuery("describe my_prometheus.prometheus_http_requests_total");
verifyColumn(
result,
columnName("TABLE_CATALOG"),
columnName("TABLE_SCHEMA"),
columnName("TABLE_NAME"),
columnName("COLUMN_NAME"),
columnName("DATA_TYPE")
);
verifyDataRows(result,
rows("my_prometheus", "default", "prometheus_http_requests_total", "handler", "keyword"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "code", "keyword"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "instance", "keyword"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "@value", "double"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "@timestamp",
"timestamp"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "job", "keyword"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,4 +71,27 @@ public void testTablesFromPrometheusCatalog() throws IOException {
"counter", "", "Counter of HTTP requests."));
}


// Moved this IT from DescribeCommandIT to segregate Datasource Integ Tests.
@Test
public void testDescribeCommandWithPrometheusCatalog() throws IOException {
JSONObject result = executeQuery("describe my_prometheus.prometheus_http_requests_total");
verifyColumn(
result,
columnName("TABLE_CATALOG"),
columnName("TABLE_SCHEMA"),
columnName("TABLE_NAME"),
columnName("COLUMN_NAME"),
columnName("DATA_TYPE")
);
verifyDataRows(result,
rows("my_prometheus", "default", "prometheus_http_requests_total", "handler", "keyword"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "code", "keyword"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "instance", "keyword"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "@value", "double"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "@timestamp",
"timestamp"),
rows("my_prometheus", "default", "prometheus_http_requests_total", "job", "keyword"));
}

}
24 changes: 12 additions & 12 deletions integ-test/src/test/java/org/opensearch/sql/sql/CsvFormatIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ public void sanitizeTest() {
String result = executeQuery(
String.format(Locale.ROOT, "SELECT firstname, lastname FROM %s", TEST_INDEX_BANK_CSV_SANITIZE), "csv");
assertEquals(
"firstname,lastname\n"
+ "'+Amber JOHnny,Duke Willmington+\n"
+ "'-Hattie,Bond-\n"
+ "'=Nanette,Bates=\n"
+ "'@Dale,Adams@\n"
+ "\",Elinor\",\"Ratliff,,,\"\n",
"firstname,lastname%n"
+ "'+Amber JOHnny,Duke Willmington+%n"
+ "'-Hattie,Bond-%n"
+ "'=Nanette,Bates=%n"
+ "'@Dale,Adams@%n"
+ "\",Elinor\",\"Ratliff,,,\"%n",
result);
}

Expand All @@ -40,12 +40,12 @@ public void escapeSanitizeTest() {
String.format(Locale.ROOT, "SELECT firstname, lastname FROM %s", TEST_INDEX_BANK_CSV_SANITIZE),
"csv&sanitize=false");
assertEquals(
"firstname,lastname\n"
+ "+Amber JOHnny,Duke Willmington+\n"
+ "-Hattie,Bond-\n"
+ "=Nanette,Bates=\n"
+ "@Dale,Adams@\n"
+ "\",Elinor\",\"Ratliff,,,\"\n",
"firstname,lastname%n"
+ "+Amber JOHnny,Duke Willmington+%n"
+ "-Hattie,Bond-%n"
+ "=Nanette,Bates=%n"
+ "@Dale,Adams@%n"
+ "\",Elinor\",\"Ratliff,,,\"%n",
result);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@ public void rawFormatWithPipeFieldTest() {
String result = executeQuery(
String.format(Locale.ROOT, "SELECT firstname, lastname FROM %s", TEST_INDEX_BANK_RAW_SANITIZE), "raw");
assertEquals(
"firstname|lastname\n"
+ "+Amber JOHnny|Duke Willmington+\n"
+ "-Hattie|Bond-\n"
+ "=Nanette|Bates=\n"
+ "@Dale|Adams@\n"
+ "@Elinor|\"Ratliff|||\"\n",
"firstname|lastname%n"
+ "+Amber JOHnny|Duke Willmington+%n"
+ "-Hattie|Bond-%n"
+ "=Nanette|Bates=%n"
+ "@Dale|Adams@%n"
+ "@Elinor|\"Ratliff|||\"%n",
result);
}

Expand Down
24 changes: 13 additions & 11 deletions scripts/integtest.sh
Original file line number Diff line number Diff line change
Expand Up @@ -95,18 +95,20 @@ fi
USERNAME=`echo $CREDENTIAL | awk -F ':' '{print $1}'`
PASSWORD=`echo $CREDENTIAL | awk -F ':' '{print $2}'`

OPENSEARCH_HOME=`ps -ef | grep -o "[o]pensearch.path.home=\S\+" | cut -d= -f2- | head -n1`

curl -SL https://raw.githubusercontent.com/opensearch-project/sql/main/integ-test/src/test/resources/datasource/datasources.json -o "$OPENSEARCH_HOME"/datasources.json

yes | $OPENSEARCH_HOME/bin/opensearch-keystore add-file plugins.query.federation.datasources.config $OPENSEARCH_HOME/datasources.json

if [ $SECURITY_ENABLED == "true" ]
OS="`uname`"
##Cygwin or MinGW packages should be preinstalled in the windows.
## This command doesn't work without bash
if [ $OS != "WindowsNT" ]
Yury-Fridlyand marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

windows doesn't have a uname or bash, I remember different uname gives different outputs on windows. which uname is this? maybe document prerequisite dependencies in case user wants to run tests on windows

Copy link
Member Author

Choose a reason for hiding this comment

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

This requires Cygwin or MinGW packages and these are installed in the ami used in our build systems. Will mention it as a comment.

then
curl -k --request POST --url https://$BIND_ADDRESS:$BIND_PORT/_nodes/reload_secure_settings --header 'content-type: application/json' --data '{"secure_settings_password":""}' --user $CREDENTIAL
else
curl --request POST --url http://$BIND_ADDRESS:$BIND_PORT/_nodes/reload_secure_settings --header 'content-type: application/json' --data '{"secure_settings_password":""}'
OPENSEARCH_HOME=`ps -ef | grep -o "[o]pensearch.path.home=\S\+" | cut -d= -f2- | head -n1`
curl -SL https://raw.githubusercontent.com/opensearch-project/sql/main/integ-test/src/test/resources/datasource/datasources.json -o "$OPENSEARCH_HOME"/datasources.json
yes | $OPENSEARCH_HOME/bin/opensearch-keystore add-file plugins.query.federation.datasources.config $OPENSEARCH_HOME/datasources.json
if [ $SECURITY_ENABLED == "true" ]
then
curl -k --request POST --url https://$BIND_ADDRESS:$BIND_PORT/_nodes/reload_secure_settings --header 'content-type: application/json' --data '{"secure_settings_password":""}' --user $CREDENTIAL
else
curl --request POST --url http://$BIND_ADDRESS:$BIND_PORT/_nodes/reload_secure_settings --header 'content-type: application/json' --data '{"secure_settings_password":""}'
fi
fi


./gradlew integTest -Dopensearch.version=$OPENSEARCH_VERSION -Dbuild.snapshot=$SNAPSHOT -Dtests.rest.cluster="$BIND_ADDRESS:$BIND_PORT" -Dtests.cluster="$BIND_ADDRESS:$BIND_PORT" -Dtests.clustername="opensearch-integrationtest" -Dhttps=$SECURITY_ENABLED -Duser=$USERNAME -Dpassword=$PASSWORD --console=plain
vamsimanohar marked this conversation as resolved.
Show resolved Hide resolved