Run JS API unit tests and integration tests in the Gradle build #4988

niloc132 · 2023-12-27T20:21:20Z

Restores some unit and integration tests to run as part of the local/CI builds, as part of the check task.

Partial #188

replace this with handling many smaller snapshots

web/client-api/src/main/java/io/deephaven/web/client/api/JsColumnStatistics.java

web/client-api/src/main/java/io/deephaven/web/client/api/WorkerConnection.java

web/client-api/src/main/java/io/deephaven/web/client/api/JsTable.java

...client-api/src/main/java/io/deephaven/web/client/api/subscription/SubscriptionTableData.java

mofojed

We should include instructions on how to run these tests as well. I ran ./gradlew gwtUnitTest and that worked correctly. Then I tried running ./gradle gwtIntegrationTest and it's just giving me a bunch of UnableToCompleteExceptions for each test:

> Task :web-client-api:gwtIntegrationTest FAILED
1 compiler directives added

io.deephaven.web.ClientIntegrationTestSuite > io.deephaven.web.client.api.subscription.ViewportTestGwt.testViewportOnUpdatingTable FAILED
    com.google.gwt.junit.JUnitFatalLaunchException at JUnitShell.java:708

io.deephaven.web.ClientIntegrationTestSuite > io.deephaven.web.client.api.subscription.ViewportTestGwt.testViewportSubsetOfColumns FAILED
    com.google.gwt.core.ext.UnableToCompleteException at JUnitShell.java:1334

io.deephaven.web.ClientIntegrationTestSuite > io.deephaven.web.client.api.subscription.ViewportTestGwt.testViewportWithNoInitialItems FAILED
    com.google.gwt.core.ext.UnableToCompleteException at JUnitShell.java:1334

web/client-api/client-api.gradle

mofojed · 2023-12-28T01:54:37Z

web/client-api/client-api.gradle

+    task.awaitStatusTimeout.set 120
+    task.checkInterval.set 100


Side note - really confusing that awaitStatusTimeout is in seconds, and checkInterval is in ms.

Agreed... can go back and rewrite those, but I vote not today. This is code taken from the upstream docker-gradle plugin, and we didn't customize it further.

mofojed · 2023-12-28T01:56:36Z

web/client-api/client-api.gradle

+}
+
+// start a grpc-api server
+String randomSuffix = UUID.randomUUID().toString();


Why are we doing this? Is this safe to do? Seems like this would mess up gradle caching?

It would yes, but we are doing this all over for naming docker containers, so that you don't accidentally collide in parallel runs, or separate checkouts, etc. Since this is just for tests, we're not concerned with "oh these tests passed last time, so I'm not even going to run them this time" for integration tests.

web/client-api/src/test/java/io/deephaven/web/client/api/AbstractAsyncGwtTestCase.java

mofojed · 2023-12-28T02:16:12Z

web/client-api/src/test/java/io/deephaven/web/client/api/AbstractAsyncGwtTestCase.java

+            String[] actual = Js.uncheckedCast(getColumnData(viewportData, column.apply(table)));
+            assertTrue("Expected " + Arrays.toString(expected) + ", found " + Arrays.toString(actual) + " in table "
+                    + table + " at state " + table.state(), Arrays.equals(expected, actual));
+        }, 2000);


Define as constant - same timeout is used in the other assertNextViewportIs method.

Actually I want them to be different-ish constants, so you can nail down which timeout failed, since it can be a pain to find them.

Yup that's fair too, though would be nice to define a base constant and just add an offset for uniqueness

mofojed · 2023-12-28T02:17:05Z

web/client-api/src/test/java/io/deephaven/web/client/api/AbstractAsyncGwtTestCase.java

+        for (int i = 0; i < code.size(); i++) {
+            final int index = i;
+            result = result.then(ignore -> {
+                delayTestFinish(4000 + index);


Constant? Unsure why 4000 was selected.

It seems that running python on a brand new server in docker can sometimes be a little slow out of the gate, so I have a "large" timeout here.

It could make sense to change the strategy entirely and just have a per-test timeout instead, and set it to a few minutes - but it is nice to fail within a few seconds of the failing operation (as we don't have event handlers for all failures, we just let timeouts expire).

Yea that's all fine and good, just as a constant you can have those details as a comment so it's clear why you want a timeout of a 4s instead of say 40s or 400ms

All of these timeouts are "worst case, sometimes the browser or dh api lags a bit, here's a reasonably sized timeout that likely can't be exceeded in reasonable circumstances". 40ms definitely isn't good enough (browser GC could kill that alone), 400ms probably isn't enough either.

We could just say "every test should take less than 5 minutes" and not worry about each operation, but historically this has made it easier to figure out which specific operation is stuck/failing.

Right - that all makes sense, now add that "why" information as a comment on that timeout.

- Increased connect timeout to 1s - Added some sugar to some of the timeouts so they're easier to identify

mofojed

Works on Mac

web/client-api/src/test/java/io/deephaven/web/client/api/NullValueTestGwt.java

devinrsmith

Talking about selenium port and direct docker IP offline.

devinrsmith · 2023-12-30T02:49:35Z

docker/registry/selenium/gradle.properties

@@ -0,0 +1,3 @@
+io.deephaven.project.ProjectType=DOCKER_REGISTRY
+deephaven.registry.imageName=selenium/standalone-firefox:4.16.1-20231219


All of the other imageName are more open-ended so that the imageId can be bumped w/ bumpImage task. Is there a reason this tag was chosen?

Selenium client has historically been firmly tied to a single release of the browser+driver version, so it rarely will work to update this to "latest" without finding out what version that happens to be, and then update the test dependencies to match that specific version. This way, the version number is clear in both places.

niloc132 added 19 commits November 2, 2023 12:12

Hacked together tests

b048f9c

Tests build, but do not pass

c05823a

bad idea: opt in to a huge payload from the server - should instead

30d1df4

replace this with handling many smaller snapshots

quick hack to decrease treemap costs - fix with non-random loading

47a68cd

spotless for ci

ea53236

fix regression in formatting longs

e8ddd50

integration tests run, fail

5f8cb54

Tests are passing in manual mode, except filter setup

a855985

Tidy up tests, disable filter tests for now

06a17a5

Add additional integration tests

354fd48

null values test

d8ad542

POtential totals table test

9df4859

Attempt at reusing code from gwt-core test

d9da1ca

Nearly working tests from gradle, with TODOs

6e6326d

Checkpoint commit, gwt test tasks pass, check fails

92e6f3a

Dirty hack to avoid esoco's assumption that compile must run after test

37639a0

Don't run gwt tests with in quick

03939d9

Remove unused build file, raise timeout on a test

0d15ac3

Merge branch 'main' into js-flight

49b9b95

niloc132 added build test NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Dec 27, 2023

niloc132 added this to the January 2024 milestone Dec 27, 2023

niloc132 requested review from mofojed and devinrsmith December 27, 2023 20:21

niloc132 commented Dec 27, 2023

View reviewed changes

niloc132 added 3 commits December 27, 2023 14:49

Clean up readability, use a registry image

0d02ff3

tidy things up to working in CI

dd54ea8

Revert SubscriptionTableData, WorkerConnection

e16cbb7

Final cleanup (?)

4af1341

niloc132 marked this pull request as ready for review December 28, 2023 01:01

niloc132 requested review from jcferretti and rcaudy as code owners December 28, 2023 01:04

mofojed requested changes Dec 28, 2023

View reviewed changes

niloc132 mentioned this pull request Dec 29, 2023

JS API should expose if a table is refreshing #4999

Merged

niloc132 added 5 commits January 2, 2024 10:07

Parameterize port and timeouts, other cleanup

43a102f

Rework networking so hopefully docker-desktop behaves

2bab42b

Add healthcheck, cleanup

d61c783

Clean up forked test runner

a11687d

Inline more wd runstyle contents

e03f3e7

niloc132 force-pushed the js-flight branch from 7711088 to e03f3e7 Compare January 2, 2024 23:03

niloc132 requested a review from mofojed January 2, 2024 23:03

mofojed and others added 2 commits January 3, 2024 11:48

Change up timeouts a bit

ab0bb30

- Increased connect timeout to 1s - Added some sugar to some of the timeouts so they're easier to identify

Increase a few more timeouts and make them unique

4413dcf

mofojed previously approved these changes Jan 3, 2024

View reviewed changes

mofojed reviewed Jan 3, 2024

View reviewed changes

web/client-api/src/test/java/io/deephaven/web/client/api/NullValueTestGwt.java Outdated Show resolved Hide resolved

Add missing basic column types

a0c4f0d

niloc132 dismissed mofojed’s stale review via a0c4f0d January 3, 2024 19:50

devinrsmith reviewed Jan 3, 2024

View reviewed changes

mofojed previously approved these changes Jan 3, 2024

View reviewed changes

Fix selenium port mapping

713007d

niloc132 dismissed mofojed’s stale review via 713007d January 9, 2024 22:12

niloc132 requested review from devinrsmith and mofojed January 9, 2024 22:12

devinrsmith approved these changes Jan 9, 2024

View reviewed changes

mofojed approved these changes Jan 10, 2024

View reviewed changes

niloc132 merged commit e7af7a1 into deephaven:main Jan 17, 2024
19 checks passed

github-actions bot locked and limited conversation to collaborators Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run JS API unit tests and integration tests in the Gradle build #4988

Run JS API unit tests and integration tests in the Gradle build #4988

niloc132 commented Dec 27, 2023

mofojed left a comment

mofojed Dec 28, 2023

niloc132 Dec 29, 2023

mofojed Dec 28, 2023

niloc132 Dec 29, 2023

mofojed Dec 28, 2023

niloc132 Dec 29, 2023

mofojed Jan 2, 2024

mofojed Dec 28, 2023

niloc132 Dec 29, 2023

mofojed Jan 2, 2024

niloc132 Jan 2, 2024

mofojed Jan 2, 2024

mofojed left a comment

devinrsmith left a comment

devinrsmith Dec 30, 2023

niloc132 Jan 9, 2024

		@@ -0,0 +1,3 @@
		io.deephaven.project.ProjectType=DOCKER_REGISTRY
		deephaven.registry.imageName=selenium/standalone-firefox:4.16.1-20231219

Run JS API unit tests and integration tests in the Gradle build #4988

Run JS API unit tests and integration tests in the Gradle build #4988

Conversation

niloc132 commented Dec 27, 2023

mofojed 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

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

mofojed left a comment

Choose a reason for hiding this comment

devinrsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment