Uses dev docker container in the dev branch. No AUTO cherry-pick #2621

ncordon · 2022-03-09T15:24:32Z

What

Fetches the dev neo4j container from TeamCity.

Why

Some tests rely on the neo4j docker container, which is not published until the database is released. Those tests were being ignored in the CI. This way we can develop more reliably.

This container will be available only for PRs coming from internal contributors.

ncordon · 2022-03-11T14:52:26Z

full/build.gradle

@@ -6,12 +6,11 @@ plugins {
    id 'maven-publish'
    id 'antlr'
    id "org.sonarqube"
-    id "org.jetbrains.kotlin.jvm" version "1.5.31"
+    id "org.jetbrains.kotlin.jvm" version "1.6.0"


We need to update this in the other maintained builds

ncordon · 2022-03-11T14:52:36Z

extra-dependencies/xls/gradle/wrapper/gradle-wrapper.properties

We need to update this in the other maintained builds

ncordon · 2022-03-11T14:52:48Z

extra-dependencies/xls/build.gradle

-    sourceCompatibility = JavaVersion.VERSION_11
-    targetCompatibility = JavaVersion.VERSION_11
+    sourceCompatibility = JavaVersion.VERSION_17
+    targetCompatibility = JavaVersion.VERSION_17


We need to update this in the other maintained branches

No I don't think so, as Neo 4.x is supposed to be run with Java 11?

Yes yes, I only left this comment as a self-reminder to review these settings in other branches

ncordon · 2022-03-11T14:53:12Z

full/build.gradle

@@ -20,7 +19,7 @@ jar {
 }

 compileKotlin {
-    kotlinOptions.jvmTarget = "1.8"
+    kotlinOptions.jvmTarget = "17"


Update this in the other branches

Yes, but in the other branches 4.2, 4.3 and 4.4, I guess you should update it to 11 rather than 17.

ncordon · 2022-03-11T14:53:22Z

full/build.gradle

    configurations.all {
        exclude group: 'org.slf4j', module: 'slf4j-nop'
+        exclude group: 'ch.qos.logback', module: 'logback-classic'
    }


Add this in the other branches

ncordon · 2022-03-11T14:53:38Z

build.gradle

        filter {
            setFailOnNoMatchingTests(false)
        }
+
+        testLogging.showStandardStreams = true
+        testLogging.exceptionFormat = 'full'


Add this lines in other branches

ncordon · 2022-03-11T14:54:18Z

.github/workflows/CI.yaml

-        run: (find ~/.gradle/caches -name "*neo4j*" -exec rm -rf {} \;) || echo "All neo4j files cleaned"
-      - name: Check procedures included in apoc full
-        run: git diff --exit-code full/src/main/resources/extended.txt || (echo "extended.txt file does not contain all changes" && exit -1)
+        run: chmod +x gradlew && ./gradlew build


Simplify these lines and make sure there's no build --continue in other branches.

ncordon · 2022-03-11T14:54:54Z

test-startup/src/test/java/StartupTest.java

+        for (var version: Neo4jVersion.values()) {
+            try (Neo4jContainerExtension neo4jContainer = createDB(version, APOC_FULL, !TestUtil.isRunningInCI())
+                    .withNeo4jConfig("dbms.transaction.timeout", "5s")) {
+
+                neo4jContainer.start();
+                assertTrue("Neo4j Instance should be up-and-running", neo4jContainer.isRunning());
+
+                Session session = neo4jContainer.getSession();
+                int procedureCount = session.run("SHOW PROCEDURES YIELD name WHERE name STARTS WITH 'apoc' RETURN count(*) AS count").peek().get("count").asInt();
+                int functionCount = session.run("SHOW FUNCTIONS YIELD name WHERE name STARTS WITH 'apoc' RETURN count(*) AS count").peek().get("count").asInt();
+                int coreCount = session.run("CALL apoc.help('') YIELD core WHERE core = true RETURN count(*) AS count").peek().get("count").asInt();
+
+                assertTrue(procedureCount > 0);
+                assertTrue(functionCount > 0);
+                assertTrue(coreCount > 0);
+            } catch (Exception ex) {
+                // if Testcontainers wasn't able to retrieve the docker image we ignore the test
+                if (TestContainerUtil.isDockerImageAvailable(ex)) {
+                    ex.printStackTrace();
+                    fail("Should not have thrown exception when trying to start Neo4j: " + ex);
+                } else if (!TestUtil.isRunningInCI()) {
+                    fail( "The docker image " + TestContainerUtil.neo4jEnterpriseDockerImageVersion + " should be available in the CI");
+                }
            }
        }
    }


Refactor this in other branches to also have tests for community version (to avoid binary incompatibilities ever again)

ncordon · 2022-03-11T14:55:28Z

full/build.gradle

@@ -48,7 +47,7 @@ dependencies {
 //    implementation group: 'commons-codec', name: 'commons-codec', version: '1.14'
    implementation group: 'com.jayway.jsonpath', name: 'json-path', version: '2.4.0'
    implementation group: 'org.hdrhistogram', name: 'HdrHistogram', version: '2.1.9'
-    implementation group: 'org.neo4j.driver', name: 'neo4j-java-driver', version: '4.0.0'
+    compileOnly group: 'org.neo4j.driver', name: 'neo4j-java-driver-slim', version: '4.4.3'


Make the driver compile only in other branches to avoid overwriting the one in the database when we plug in apoc

What do you mean with 'only in other branches'? I'm not sure I follow...

Sorry, I'll word it in other way: I want the version of java driver to be a compileOnly dependency as well in other branches (since it is included in the database already). Not doing this causes the database to evict its own java version and use the one included in the apoc jar.

ncordon · 2022-03-11T15:04:48Z

core/src/test/java/apoc/StartupTest.java

@@ -20,7 +20,7 @@
    public void test() {
        try {
            Neo4jContainerExtension neo4jContainer = createEnterpriseDB(!TestUtil.isRunningInCI())
-                    .withNeo4jConfig("dbms.transaction.timeout", "5s");
+                    .withNeo4jConfig("dbms.transaction.timeout", "60s");


Port this to other branches

Lojjs

Most things looks good, just some smaller comments and questions

.github/workflows/CI.yaml

Lojjs · 2022-03-14T10:01:27Z

extra-dependencies/xls/build.gradle

-    sourceCompatibility = JavaVersion.VERSION_11
-    targetCompatibility = JavaVersion.VERSION_11
+    sourceCompatibility = JavaVersion.VERSION_17
+    targetCompatibility = JavaVersion.VERSION_17


No I don't think so, as Neo 4.x is supposed to be run with Java 11?

Lojjs · 2022-03-14T10:04:02Z

full/build.gradle

@@ -20,7 +19,7 @@ jar {
 }

 compileKotlin {
-    kotlinOptions.jvmTarget = "1.8"
+    kotlinOptions.jvmTarget = "17"


Yes, but in the other branches 4.2, 4.3 and 4.4, I guess you should update it to 11 rather than 17.

full/src/main/java/apoc/bolt/Bolt.java

Lojjs · 2022-03-14T10:07:13Z

full/build.gradle

@@ -48,7 +47,7 @@ dependencies {
 //    implementation group: 'commons-codec', name: 'commons-codec', version: '1.14'
    implementation group: 'com.jayway.jsonpath', name: 'json-path', version: '2.4.0'
    implementation group: 'org.hdrhistogram', name: 'HdrHistogram', version: '2.1.9'
-    implementation group: 'org.neo4j.driver', name: 'neo4j-java-driver', version: '4.0.0'
+    compileOnly group: 'org.neo4j.driver', name: 'neo4j-java-driver-slim', version: '4.4.3'


What do you mean with 'only in other branches'? I'm not sure I follow...

test-startup/src/test/java/StartupTest.java

ncordon changed the title ~~Uses dev docker container in the dev branch~~ Uses dev docker container in the dev branch. No AUTO cherry-pick Mar 9, 2022

ncordon added 5.0 team-cypher-surface Cypher Surface team should review the PR labels Mar 9, 2022

ncordon assigned Lojjs Mar 10, 2022

ncordon force-pushed the dev-use-dev-docker branch 5 times, most recently from 6952d1a to 91045d7 Compare March 10, 2022 15:52

ncordon added the not-ready-for-merge label Mar 11, 2022

ncordon added 4 commits March 11, 2022 11:46

Uses neo4j dev docker container

d5007de

Deletes runtime dependency and makes container tests fail properly

022f115

Hardens the tests adding support for community

d96bf77

Tries to download both community and enterprise docker

b8095ca

ncordon force-pushed the dev-use-dev-docker branch from 91045d7 to b8095ca Compare March 11, 2022 11:46

ncordon added 2 commits March 11, 2022 14:44

Produce normal sized output logs

18db6ab

Merge remote-tracking branch 'origin/dev' into dev-use-dev-docker

41bf66f

ncordon commented Mar 11, 2022

View reviewed changes

Increases timeouts for docker

096bbf7

ncordon commented Mar 11, 2022

View reviewed changes

Solves the issue with some procedures not being marked as full

70b8b8f

ncordon removed the not-ready-for-merge label Mar 14, 2022

Lojjs requested changes Mar 14, 2022

View reviewed changes

ncordon added 2 commits March 14, 2022 11:51

Removes more warnings

3d4da98

Addresses review comments

a2199b5

Lojjs approved these changes Mar 14, 2022

View reviewed changes

ncordon merged commit f896da6 into dev Mar 14, 2022

ncordon deleted the dev-use-dev-docker branch March 14, 2022 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uses dev docker container in the dev branch. No AUTO cherry-pick #2621

Uses dev docker container in the dev branch. No AUTO cherry-pick #2621

ncordon commented Mar 9, 2022

ncordon Mar 11, 2022 •

edited

Loading

ncordon Mar 11, 2022 •

edited

Loading

ncordon Mar 11, 2022

Lojjs Mar 14, 2022

ncordon Mar 14, 2022

ncordon Mar 11, 2022

Lojjs Mar 14, 2022

ncordon Mar 11, 2022

ncordon Mar 11, 2022

ncordon Mar 11, 2022

ncordon Mar 11, 2022

ncordon Mar 11, 2022

Lojjs Mar 14, 2022

ncordon Mar 14, 2022

ncordon Mar 11, 2022

Lojjs left a comment

Lojjs Mar 14, 2022

Lojjs Mar 14, 2022

Lojjs Mar 14, 2022

Uses dev docker container in the dev branch. No AUTO cherry-pick #2621

Uses dev docker container in the dev branch. No AUTO cherry-pick #2621

Conversation

ncordon commented Mar 9, 2022

What

Why

ncordon Mar 11, 2022 • edited Loading

Choose a reason for hiding this comment

ncordon Mar 11, 2022 • 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

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

Lojjs 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

ncordon Mar 11, 2022 •

edited

Loading

ncordon Mar 11, 2022 •

edited

Loading