Use testcontainers to improve platform coverage #408

pilosus · 2023-11-21T22:10:25Z

About

Use "Testcontainers" as framework for providing service fixtures.

References

Use CrateDB test layer based on "Testcontainers", in order to improve platform coverage #402
Adapt testcontainers to be testing framework agnostic cratedb-toolkit#82

Checklist

CLA is signed

amotl

Dear Vitaly,

this looks looks excellent, thank you very much.

CI says something about this:

ERROR: Could not find a version that satisfies the requirement click-aliases<2,>=1.0.3 (from cratedb-toolkit[test]) (from versions: 1.0.0, 1.0.1)
ERROR: No matching distribution found for click-aliases<2,>=1.0.3

-- https://github.com/crate/crash/actions/runs/6950117802/job/18922855944#step:4:124

... but that is totally not your fault. We recently needed to add crate/cratedb-toolkit@0a16a101cb45, and that will probably need some rebalancing we will take care about on behalf of the next iteration.

With kind regards,
Andreas.

pilosus · 2023-11-22T11:16:37Z

@amotl yes, this is because of the Python version constraints in click-aliases. But the cratedb-toolkit itself has the same constraint of >=3.8, we simply don't see it, as the pip install fails before all the transitive deps are installed.
Tbh, I think Python 3.7 constraines can be lifted to 3.8 safely:

Python 3.7 users are almost non-existent in the crash download stats
Python 3.7 has reached the end of life this summer

amotl · 2023-11-22T21:17:00Z

Python 3.7 has reached the end of life this summer

urllib3 also just removed support, see https://github.com/urllib3/urllib3/releases/tag/2.1.0.

pilosus · 2023-11-22T23:45:12Z

setup.py

@@ -76,7 +76,9 @@ def read(path):
    extras_require=dict(
        test=[
            'crate[test]',
-            'zc.customdoctests<2'
+            'zc.customdoctests<2',
+            # FIXME once tested and merged, fix the name


NB! Needs to be updated before the merge

Thank you. See also #410 (comment):

@pilosus: When following the (in my opinion) right strategy outlined at 2., I am humbly asking for your patience on this. It would need to wait a bit for the testcontainers code to be upstreamed beforehand, [...]

[ on which you have worked on behalf of crate/cratedb-toolkit#82 ].

We observe that the test adapter may need a few more improvements, so we postponed the plan about the upstreaming. We released the most recent updates with cratedb-toolkit 0.0.4. Can I ask you to upgrade, and, while being at it, also rebase on top of master?

pilosus · 2023-11-22T23:57:31Z

@amotl I think I am ready for the review, please let me know if I missed something.

amotl · 2023-11-23T00:01:41Z

tests/test_integration.py



+node = CrateDBFixture(crate_version=EntrypointOpts.version)


With respect to your question on the other patch, could this spot also be used to optionally register table names for resetting, on behalf of a second argument to the fixture manager here? In this way, it can be managed by CrateDBFixture, but the user has control about which tables need to be defined here, matching their test environment.

The list of tables could be supplied to the constructor, or optionally by using a dedicated method, or both. It is just an idea, I don't have any strong opinions about the "how", and I am also sure you will find a better way of "naming things". ;]

node.register_tables_for_reset(*tablenames: t.List[str])

node = CrateDBFixture( crate_version=EntrypointOpts.version, reset_tables=["foo", "bar", '"testdrive"."qux"'], )

You already responded to this at crate/cratedb-toolkit#82 (comment) while I was writing this down. Your proposed interface is perfectly lean. Thanks!

cratedb_service.reset(tables=RESET_TABLES)

Yes, I think passing the table list to the reset method is even better than using it in the class constructor/class method, as you can adjust the list with more flexibility with no need to change the main fixture.

tests/test_integration.py

matriv · 2023-11-24T09:47:15Z

tests/test_integration.py

@@ -286,7 +292,7 @@ def test_multiple_hosts(self):
                    output = output.getvalue()
                    lines = output.split('\n')
                    self.assertRegex(lines[3], r'^\| http://[\d\.:]+ .*\| NULL .*\| FALSE .*\| Server not available')
-                    self.assertRegex(lines[4], r'^\| http://[\d\.:]+. *\| crate .*\| TRUE .*\| OK')
+                    self.assertRegex(lines[4], r'^\| http://.*@?localhost:\d+ *\| crate .*\| TRUE .*\| OK')


Is this always localhost because of testcontainers now?

it really depends on two things:

if you pass host explicitly or rely on the default None value, when invoking get_connection_url method of the CrateDBTestAdapter

where the code is running, on the host or Docker-in-Docker, see the logic here. In our case, both for local development and in the CI pipelines, it will always be localhost

so, I'd say, it's safe to assume localhost won't break anything, unless testcontainers upstream or CI/CD pipelines in crash repo will be changed

I see, thx!

matriv

Thank you! Please wait for other reviewers though.

seut

Nice work and thank you a lot for your efforts @pilosus!
But related to the discussion we had about adding dependencies to incubating projects and the future of the testcontainers, please do not merge this until the testcontainer is merged upstream.
See #410 (comment).

amotl · 2024-01-19T20:32:53Z

Hi.

I just invoked the test harness, and while it succeeds:

python -m unittest -v
Ran 103 tests in 8.408s

.... it clutters the log with such messages:

2024-01-19 21:30:31,578 [urllib3.connectionpool              ] WARNING : Retrying (Retry(total=8, connect=None, read=0, redirect=None, status=None)) after connection broken by 'NewConnectionError('<urllib3.connection.HTTPConnection object at 0x113ba4d50>: Failed to establish a new connection: [Errno 61] Connection refused')': /
2024-01-19 21:30:31,579 [urllib3.connectionpool              ] WARNING : Retrying (Retry(total=7, connect=None, read=0, redirect=None, status=None)) after connection broken by 'NewConnectionError('<urllib3.connection.HTTPConnection object at 0x113b4f590>: Failed to establish a new connection: [Errno 61] Connection refused')': /
2024-01-19 21:30:31,580 [urllib3.connectionpool              ] WARNING : Retrying (Retry(total=6, connect=None, read=0, redirect=None, status=None)) after connection broken by 'NewConnectionError('<urllib3.connection.HTTPConnection object at 0x113b3eed0>: Failed to establish a new connection: [Errno 61] Connection refused')': /

There was also another error on a single build, at »Python 3.12, CrateDB 5.5.0 on macos-latest«:

  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1025, in send
    self.connect()
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/site-packages/docker/transport/unixconn.py", line 27, in connect
    sock.connect(self.unix_socket)
FileNotFoundError: [Errno 2] No such file or directory

Do you know where this might be coming from, @pilosus?

With kind regards,
Andreas.

pilosus · 2024-01-21T15:21:54Z

.... it clutters the log with such messages:

Hi @amotl

My educated guess is that since the PR was open, a new major version of docker was released. It has requests package bumped to a new version, which in turns uses urllib3 of version 2 distributed rather than vendored as it used to be in earlier versions. Here's a longer discussion:
docker/docker-py#3113

Since testcontainers use docker package with no pinned upper bound, the behaviour from requests has changed in crash too.

From what I see, it's not only testcontainers-related tests that are emitting these annoying warnings, but some others, like tests/test_commands.py since they are instantiating crate.crash.command.CrateShell which is invoking crate.crash.command.CrateShell._connect.

I think it's better to use either:

urllib3.disable_warnings()

or a module-wide fixtures like:

def setUpModule():
    import logging
    logging.getLogger("urllib3").propagate = False

Python 3.12, CrateDB 5.5.0 on macos-latest

Not sure what's happening here, but again, probably it has something to do with the new docker-py.

pilosus · 2024-01-21T15:59:59Z

@amotl reg. MacOS tests failing. It's because Docker isn't pre-installed on the GitHub-runners!
https://github.com/orgs/community/discussions/25777

If you open a job, then click on Set up job -> Runner Image -> Included Software, you'll see the link to a list of preinstalled runner software.
Compare ubuntu with macos-12, docker is not installed because of the licensing issues.

So, testcontainers won't work on MacOS runner, unfortunately :-/

amotl · 2024-02-07T16:48:26Z

@pilosus:

reg. MacOS tests failing. It's because Docker isn't pre-installed on the GitHub-runners!

Yeah right, thanks for the reminder. That's also why the code skip integration tests on all other OS than Linux, so it will fortunately not be a regression.

crash/tests/test_integration.py

Lines 28 to 29 in 78cf682

    
           if sys.platform != "linux": 
        
               raise SkipTest("Integration tests only supported on Linux")

Apparently, the raise SkipTest(...) would need to (additionally?) go into setUpModule() now?

crash/tests/test_integration.py

Lines 49 to 50 in 78cf682

    
           def setUpModule(): 
        
               node.start()

Otherwise, it apparently tries to dial the Docker API, which goes south.

amotl · 2024-02-07T16:56:18Z

.... it clutters the log with such messages:

My educated guess is that since the PR was open, a new major version of docker was released. It has requests package bumped to a new version, which in turns uses urllib3 of version 2 distributed rather than vendored as it used to be in earlier versions. Here's a longer discussion: [...]

Oh wow, thanks for digging into this. I hadn't expected that. Instead, I was expecting that the CrateDB HTTP driver might have a flaw, and is responsible for the log flodding coming from urllib3. Without reading docker/docker-py#3113 yet, can you rule out that the HTTP driver needs to be investigated, and that it's definitively coming from Docker?

If so, I will be happy to disable corresponding warnings as you suggested. However, it would be nicer if this could only be masked for requests to the Docker API, if that is the culprit here, in order not to obfuscate any other warnings originating at urllib3. Do you see a chance to achieve this?

pilosus · 2024-02-10T21:30:29Z

Yeah right, thanks for the reminder. That's also why the code skip integration tests on all other OS than Linux, so it will fortunately not be a regression.

crash/tests/test_integration.py

Lines 28 to 29 in 78cf682

if sys.platform != "linux":

raise SkipTest("Integration tests only supported on Linux")

Apparently, the raise SkipTest(...) would need to (additionally?) go into setUpModule() now?

@amotl Check out how I've impemented it in d83177f

pilosus · 2024-02-10T21:37:01Z

Oh wow, thanks for digging into this. I hadn't expected that. Instead, I was expecting that the CrateDB HTTP driver might have a flaw, and is responsible for the log flodding coming from urllib3. Without reading docker/docker-py#3113 yet, can you rule out that the HTTP driver needs to be investigated, and that it's definitively coming from Docker?

If so, I will be happy to disable corresponding warnings as you suggested. However, it would be nicer if this could only be masked for requests to the Docker API, if that is the culprit here, in order not to obfuscate any other warnings originating at urllib3. Do you see a chance to achieve this?

@amotl hmm, all this flood of urllib3 warning logs are coming from the CrateShell when we forgot pass in proper hosts so that it falls back to the default localhost:4200. I've fixed that in 58b0aef, except (obviously) for the case where we check multi hosts connection with one URL not available.

There's the same problem in some other, non-integration tests like tests.test_commands.ShowTablesCommandTest.test_pre_0_57 and tests.test_commands.MultipleStatementsTest.test_single_sql_statement_multiple_lines just to name a few. It seems we don't mock them properly. I haven't fixed those, I guess it's better to do in a separate PR.

amotl · 2024-02-10T23:12:42Z

Thank you very much for improving the test skipping on unsupported OS, and for tracking down the reason for the flood of urllib3 warnings, and fixing it. Excellent job! 💯

pilosus · 2024-03-03T13:03:19Z

Hi @amotl let me know if there's anything else I need to do to get this PR merged?
If there's a blocker may be it's worth to mention it here as well :-)

amotl · 2024-03-03T16:04:16Z

Thanks @pilosus. I think it will be good to go. Let us have @seut a final voice about it, and then we will probably just merge, when no other objections come up. Thanks again for your efforts!

seut

Nice, thx!
I think some commits would makes sense to be squashed, but will leave that up to you.

GitHub Runner don't support Docker on MacOS for licensing issues. We want to skip integration tests that require Docker though testcontainers. We still may want to be able to run such test for local development.

CrateShell invocation in integration tests relied on the default localhost:4200 instead of the custom host urls used in Docker containers. That resulted in a flood of urllib3 warning logs coming from the crate client. This change fix the hosts urls.

pilosus · 2024-03-05T22:28:11Z

I think some commits would makes sense to be squashed, but will leave that up to you.

@amotl @seut I've squashed 6 commits. Feel free to squash even more if needed.

amotl reviewed Nov 22, 2023

View reviewed changes

pilosus mentioned this pull request Nov 22, 2023

Adapt testcontainers to be testing framework agnostic crate/cratedb-toolkit#82

Merged

1 task

pilosus changed the title ~~Feature/402 testcontainers for integration testing~~ Use testcontainers to improve plaform coverage Nov 22, 2023

amotl mentioned this pull request Nov 22, 2023

Remove QA support for Python 3.7 #410

Closed

pilosus force-pushed the feature/402-testcontainers-for-integration-testing branch from cc142eb to 0446f7d Compare November 22, 2023 23:44

pilosus commented Nov 22, 2023

View reviewed changes

pilosus marked this pull request as ready for review November 22, 2023 23:45

pilosus requested a review from amotl November 22, 2023 23:57

amotl reviewed Nov 23, 2023

View reviewed changes

pilosus requested a review from amotl November 23, 2023 13:41

amotl changed the title ~~Use testcontainers to improve plaform coverage~~ Use testcontainers to improve platform coverage Nov 23, 2023

amotl requested review from seut and matriv November 23, 2023 20:05

amotl requested changes Nov 23, 2023

View reviewed changes

tests/test_integration.py Outdated Show resolved Hide resolved

matriv reviewed Nov 24, 2023

View reviewed changes

pilosus force-pushed the feature/402-testcontainers-for-integration-testing branch from bff8dac to 7e20b38 Compare November 24, 2023 10:48

pilosus requested review from matriv and amotl November 24, 2023 10:48

matriv approved these changes Nov 24, 2023

View reviewed changes

amotl mentioned this pull request Nov 24, 2023

CI: Turn off failing fast on the test matrix slots #413

Merged

seut requested changes Nov 24, 2023

View reviewed changes

amotl force-pushed the feature/402-testcontainers-for-integration-testing branch from 7e20b38 to 4ab569c Compare January 19, 2024 20:33

amotl mentioned this pull request Jan 19, 2024

Packaging: Use cloud extra to install relevant packages crate/cratedb-toolkit#101

Merged

amotl force-pushed the feature/402-testcontainers-for-integration-testing branch 2 times, most recently from 7e55274 to 2ce2313 Compare January 19, 2024 21:59

This was referenced Feb 10, 2024

[Testing] Demonstrate CrateDB test layers with parameterization crate/cratedb-examples#282

Open

Testing: Demonstrate Python test layer implementations crate/cratedb-examples#280

Merged

pilosus force-pushed the feature/402-testcontainers-for-integration-testing branch from 7dba3fc to b87f929 Compare February 11, 2024 08:12

amotl approved these changes Mar 3, 2024

View reviewed changes

amotl requested a review from seut March 3, 2024 16:05

seut approved these changes Mar 5, 2024

View reviewed changes

pilosus and others added 6 commits March 5, 2024 23:21

Use testcontainers for integration testing

cadc71f

Update changelog

00c97a1

Dependencies: Update cratedb-toolkit

29e7300

fix: check if integration tests can be run, skip if they cannot

da28d89

GitHub Runner don't support Docker on MacOS for licensing issues. We want to skip integration tests that require Docker though testcontainers. We still may want to be able to run such test for local development.

chore: use canonical name for cratedb-toolkit package

4640109

pilosus force-pushed the feature/402-testcontainers-for-integration-testing branch from b87f929 to 4640109 Compare March 5, 2024 22:26

amotl merged commit eb4c386 into crate:master Mar 18, 2024
20 checks passed

Annnnnnnnki approved these changes Sep 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use testcontainers to improve platform coverage #408

Use testcontainers to improve platform coverage #408

pilosus commented Nov 21, 2023 •

edited

Loading

amotl left a comment

pilosus commented Nov 22, 2023 •

edited

Loading

amotl commented Nov 22, 2023

pilosus Nov 22, 2023

amotl Nov 23, 2023

amotl Feb 10, 2024 •

edited

Loading

pilosus Feb 11, 2024

pilosus commented Nov 22, 2023

amotl Nov 23, 2023 •

edited

Loading

amotl Nov 23, 2023 •

edited

Loading

pilosus Nov 23, 2023

matriv Nov 24, 2023

pilosus Nov 24, 2023

matriv Nov 24, 2023

matriv left a comment

seut left a comment

amotl commented Jan 19, 2024 •

edited

Loading

pilosus commented Jan 21, 2024

pilosus commented Jan 21, 2024

amotl commented Feb 7, 2024 •

edited

Loading

amotl commented Feb 7, 2024 •

edited

Loading

pilosus commented Feb 10, 2024

pilosus commented Feb 10, 2024

amotl commented Feb 10, 2024

pilosus commented Mar 3, 2024

amotl commented Mar 3, 2024 •

edited

Loading

seut left a comment

pilosus commented Mar 5, 2024

Use testcontainers to improve platform coverage #408

Use testcontainers to improve platform coverage #408

Conversation

pilosus commented Nov 21, 2023 • edited Loading

About

References

Checklist

amotl left a comment

Choose a reason for hiding this comment

pilosus commented Nov 22, 2023 • edited Loading

amotl commented Nov 22, 2023

pilosus Nov 22, 2023

Choose a reason for hiding this comment

amotl Nov 23, 2023

Choose a reason for hiding this comment

amotl Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

pilosus Feb 11, 2024

Choose a reason for hiding this comment

pilosus commented Nov 22, 2023

amotl Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

amotl Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

pilosus Nov 23, 2023

Choose a reason for hiding this comment

matriv Nov 24, 2023

Choose a reason for hiding this comment

pilosus Nov 24, 2023

Choose a reason for hiding this comment

matriv Nov 24, 2023

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

seut left a comment

Choose a reason for hiding this comment

amotl commented Jan 19, 2024 • edited Loading

pilosus commented Jan 21, 2024

pilosus commented Jan 21, 2024

amotl commented Feb 7, 2024 • edited Loading

amotl commented Feb 7, 2024 • edited Loading

pilosus commented Feb 10, 2024

pilosus commented Feb 10, 2024

amotl commented Feb 10, 2024

pilosus commented Mar 3, 2024

amotl commented Mar 3, 2024 • edited Loading

seut left a comment

Choose a reason for hiding this comment

pilosus commented Mar 5, 2024

pilosus commented Nov 21, 2023 •

edited

Loading

pilosus commented Nov 22, 2023 •

edited

Loading

amotl Feb 10, 2024 •

edited

Loading

amotl Nov 23, 2023 •

edited

Loading

amotl Nov 23, 2023 •

edited

Loading

amotl commented Jan 19, 2024 •

edited

Loading

amotl commented Feb 7, 2024 •

edited

Loading

amotl commented Feb 7, 2024 •

edited

Loading

amotl commented Mar 3, 2024 •

edited

Loading