Replaced python container launcher with GHA service

[joshthoward]

Was able to remove a couple hundred lines of integration_tests/conftest.py by making use of GitHub Actions Service feature. I think it's cleaner with the change.

[hashhar]

Looks good % extract trino version to a constant somewhere so that it's easier to know where to change it when running tests against a different version.

[joshthoward]

@hashhar just pushed it as a global variable in integration_tests/test_dbapi.py. LMK if you think that I should instead pass it in as an environment variable... I am on the fence.

[hashhar]

The global var looks good to me, I don't think we need an ENV var yet. The only reason to change the version could be to reproduce a bug with an older version or to test with a newer version.

[findepi]

Does it affect running tests from an IDE like PyCharm? How?

[joshthoward]

IDK how this would affect an IDE. The same tests are ran, but they assume that you start up a Trino container before running. So you might have an extra step, but integration test runs are way faster given that they don't have to pull down and run a container each time.

Timing it locally, I got roughly a 5x improvement.

# jh/test-refactor
pytest integration_tests  1.12s user 0.15s system 25% cpu 5.003 total
# master
pytest integration_tests  1.36s user 0.27s system 6% cpu 26.703 total

[findepi]

In Trino we have two kinds of integrations tests

• product tests, where we have separate environment setup and test execution. You can some of these test from IDE provided you start environment manually.
• "unit tests", where the test is responsible for provisioning all the necessary resources. These tests can be run from IDE with a click.

The second type of the tests is usually regarded as simple and easy to use, while the first one is regarded as hard to use and work with.

Are we basically converting "unit tests" into "product tests" here?

[joshthoward]

I see where you are going, but it's not a fair comparison. The tradeoffs are below.

Pros:

• 5x faster per run
• Less code
• Declarative API for integration test requirements

Cons:

• Must run Trino outside of IDE before running integration tests through IDE

[dungdm93]

Hi @joshthoward, is there any way to mount trino configs (e.g catalog files) to that service?

[joshthoward]

You can add volume mounts, but I would probably build a separate container for testing if needed. Tests should rely on TPC-H tiny if possible.

[ebyhr]

test_eager_loading_cursor_description will fail after rebase on upstream. Version up (351 → 355) looks unrelated for the failure, but I didn't investigate the detail.

[hashhar]

Looks good % unresolved comments.

However I do think we'll end up back at code-defined environments once we start adding integration tests for things like auth mechanisms etc. We'll think about it when we get there.

[joshthoward]

GH is deciding not to accept the second half of this commit... SMH

% git diff 9ae518f688112f644bdab40a13896fb47bf581fe
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 4349464..db971a5 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -7,7 +7,7 @@ jobs:
     runs-on: ubuntu-latest
     services:
       trino:
-        image: trinodb/trino:355
+        image: trinodb/trino:360
         ports:
           - 8080:8080
     strategy:
diff --git a/integration_tests/test_dbapi.py b/integration_tests/test_dbapi.py
index 8ba840c..474b581 100644
--- a/integration_tests/test_dbapi.py
+++ b/integration_tests/test_dbapi.py
@@ -24,7 +24,7 @@ from trino.transaction import IsolationLevel

 HOST = 'localhost'
 PORT = 8080
-TRINO_VERSION = '355'  # The Trino server version used for integration tests
+TRINO_VERSION = '360'  # The Trino server version used for integration tests

[joshthoward]

@ebyhr, @hashhar should be good to go. LMK if you have any further comments.

[hashhar]

Turns out we'll need the ability to have programmatic integration test environments since we want to test a matrix of versions now to make sure newer clients are compatible with older ones (and different server versions).

@joshthoward I'm closing this accordingly.