Testflows -> Pytest #454

slvrtrn · 2024-09-13T21:19:08Z

Refactored most of the tests from testflows to pytest, which is much more convenient to run from the PyCharm IDE and, hopefully, clearer
Added pytest run to the CI
Removed all python stuff from CMakeLists
Added a docker-compose file with ClickHouse
Minimal CONTRIBUTING.md (to be updated in the follow-ups)

TODO (follow-ups):

Tests will nullable types - see FIXME in the code, None binds as '' and the rendered queries have weird casts to String/LowCardinality(String).

Sample error:

Attempt to read after eof: while converting '' to UInt8. (ATTEMPT_TO_READ_AFTER_EOF)

Sample rendered query:

SELECT * FROM test_sanity_simple_data_types 
WHERE ni = _CAST(NULL, 'Nullable(String)') 
ORDER BY i ASC, s ASC, d ASC

instead of

SELECT * FROM test_sanity_simple_data_types
WHERE ni = NULL 
ORDER BY i ASC, s ASC, d ASC

which is still incorrect, though. Should be actually rendered as:

SELECT * FROM test_sanity_simple_data_types
WHERE isNull(ni)
ORDER BY i ASC, s ASC, d ASC

Salvage the useful tests from the root test.py and test.sh files and make them using pytest, too
Cleanup the test dir

alesapin · 2024-09-16T13:16:25Z

#452 (comment) Thank you, please replace them all with pytest.

slvrtrn · 2024-09-17T19:26:07Z

test/src/e2e/test_datatypes.py

+#  Array
+#  Tuple
+#  Map
+#  LowCardinality


Note that in the deleted regression.py, nullable tests are also marked as expected failure.
However, most of the data types tests that were "not supported" actually work, and implemented now.

slvrtrn · 2024-09-17T19:26:42Z

test/docker/Dockerfile

@@ -30,9 +30,6 @@ RUN apt-get update -y \
        libssl-dev \


Probably this Dockerfile should be removed entirely (in the follow-up).

slvrtrn · 2024-09-17T19:27:43Z

driver/test/datetime_it.cpp

            "toDateTime64('2020-03-25 12:11:22.123456789', 9, 'Asia/Kathmandu')", SQL_TYPE_TIMESTAMP,
-            "2020-03-25 09:26:22.123456789", SQL_TIMESTAMP_STRUCT{2020, 3, 25, 9, 26, 22, 123456789}
+            "2020-03-25 06:26:22.123456789", SQL_TIMESTAMP_STRUCT{2020, 3, 25, 6, 26, 22, 123456789}


adjusted for the clickhouse server running in docker with default UTC timezone

slvrtrn · 2024-09-17T19:28:24Z

.github/workflows/Docker.yml

@@ -1,100 +0,0 @@
-name: Build and Test - Docker


Removed as I am not sure what is the point of this. The tests are executed without this test runner.

slvrtrn · 2024-09-17T19:29:17Z

test/test.py

@@ -16,6 +16,7 @@
 is_python_3 = (sys.version_info.major == 3)
 is_windows = (os.name == 'nt')

+


to be revisited and rewritten in pytest in a follow-up, if necessary

the same with test.sh - it executes a lot of queries, but never asserts the output.

slvrtrn · 2024-09-17T19:30:07Z

.github/workflows/Linux.yml

@@ -107,7 +100,7 @@ jobs:
        -DTEST_DSN_LIST="ClickHouse DSN (ANSI);ClickHouse DSN (Unicode);ClickHouse DSN (ANSI, RBWNAT)"

    - name: Build
-      run: cmake --build ${{ github.workspace }}/build --config ${{ matrix.build_type }}
+      run: cmake --build ${{ github.workspace }}/build --config ${{ matrix.build_type }} --parallel $(nproc)


speeds up the build by a few minutes

slvrtrn · 2024-09-17T19:30:24Z

.github/workflows/Linux.yml

@@ -122,94 +115,96 @@ jobs:
    # However, these binaries are uploaded to be available in GH's 'Actions', just in case.
    - name: Upload the artifacts
      if:  ${{ matrix.compiler == 'GCC' && matrix.odbc_provider == 'UnixODBC' && matrix.build_type == 'Release' && matrix.runtime_link == 'dynamic-runtime' && matrix.third_parties == 'bundled-third-parties' }}
-      uses: actions/upload-artifact@v3
+      uses: actions/upload-artifact@v4


fixed outdated Node.js warning

slvrtrn · 2024-09-17T19:30:47Z

.github/workflows/Linux.yml

-        docker pull ${CLICKHOUSE_SERVER_IMAGE}
-        docker run -d --name clickhouse ${CLICKHOUSE_SERVER_IMAGE}
-        docker ps -a
-        docker stats -a --no-stream


The CI uses docker-compose for that now, so it's more convenient locally as well.

slvrtrn · 2024-09-17T19:32:14Z

.github/workflows/Linux.yml

+      run: |
+        export ODBCSYSINI=
+        export ODBCINSTINI="${{ github.workspace }}/run/.odbcinst.ini"
+        export ODBCINI="${{ github.workspace }}/run/.odbc.ini"


Probably should have been made a bit cleaner and extracted into some "generate env variables" step, but it works. Note the empty ODBCSYSINI (mentioned in the contribution guide now, too).

driver/test/statement_parameters_it.cpp

slvrtrn · 2024-09-17T19:34:42Z

test/parameterized/regression.py

-    Feature(run=parameterized, flags=TE)
-
-if main():
-    xfails = {


Note the expected failures here.

test/src/e2e/test_datatypes.py

slvrtrn · 2024-09-17T19:37:11Z

It should be "mergeable," I think. Added some comments to make the review a bit easier.

mshustov · 2024-09-19T08:30:56Z

driver/test/datetime_it.cpp

            "toDateTime64('2020-03-25 12:11:22.123456789', 9, 'Asia/Kathmandu')", SQL_TYPE_TIMESTAMP,
-            "2020-03-25 09:26:22.123456789", SQL_TIMESTAMP_STRUCT{2020, 3, 25, 9, 26, 22, 123456789}
+            "2020-03-25 06:26:22.123456789", SQL_TIMESTAMP_STRUCT{2020, 3, 25, 6, 26, 22, 123456789}
        }/*,

        // TODO: uncomment once the target ClickHouse server is 21.4+


should be uncommented now?

it still does not work :)

mshustov · 2024-09-19T08:33:04Z

test/src/e2e/test_datatypes.py

+#  After the fix, Nullable test cases should be re-added
+#  Sample error: Attempt to read after eof: while converting '' to UInt8. (ATTEMPT_TO_READ_AFTER_EOF)
+#
+# TODO:


Should we create a separate task to track these test improvements?

slvrtrn · 2024-09-19T09:16:03Z

@alesapin, can you please confirm that

it works on your machine
CMakeLists changes are ok (there are no more Python invocations there)

After that, we can merge the branch.

alesapin

If it's possible let's fix timezone issue.

Move most of the tests to pytest

7615330

slvrtrn added 12 commits September 16, 2024 19:24

Remove python tests from CMake, fix C++ IT, adjust Linux CI

f4fb903

Update Linux CI

e334065

Update Linux CI

ff58436

Remove testflows imports

c00edd4

Update Linux CI

af7bccd

Update Linux CI

b1d9615

Update Linux CI

d98ad5c

Full matrix for Linux CI, move Python tests to the end

80994f3

Remove Docker workflow

30a7b53

An attempt to fix iODBC pytest CI

6389d32

Move CH docker up in the workflow to give it some time to start properly

c59e14c

Update CONTRIBUTING.md, bump workflow versions

8e4ae0d

slvrtrn marked this pull request as ready for review September 17, 2024 17:39

slvrtrn requested review from mshustov and alesapin September 17, 2024 19:23

slvrtrn commented Sep 17, 2024

View reviewed changes

driver/test/statement_parameters_it.cpp Show resolved Hide resolved

slvrtrn commented Sep 17, 2024

View reviewed changes

test/src/e2e/test_datatypes.py Show resolved Hide resolved

mshustov approved these changes Sep 19, 2024

View reviewed changes

alesapin approved these changes Oct 1, 2024

View reviewed changes

Remove the failing assertion

1178eba

slvrtrn merged commit 1996aba into master Oct 2, 2024
38 of 46 checks passed

slvrtrn deleted the pytest branch October 2, 2024 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testflows -> Pytest #454

Testflows -> Pytest #454

slvrtrn commented Sep 13, 2024 •

edited

Loading

alesapin commented Sep 16, 2024

slvrtrn Sep 17, 2024

slvrtrn Sep 17, 2024 •

edited

Loading

slvrtrn Sep 17, 2024

slvrtrn Sep 17, 2024

slvrtrn Sep 17, 2024 •

edited

Loading

slvrtrn Sep 17, 2024

slvrtrn Sep 17, 2024

slvrtrn Sep 17, 2024 •

edited

Loading

slvrtrn Sep 17, 2024

slvrtrn Sep 17, 2024

slvrtrn commented Sep 17, 2024

mshustov Sep 19, 2024

slvrtrn Sep 19, 2024

mshustov Sep 19, 2024

slvrtrn commented Sep 19, 2024

alesapin left a comment

		@@ -16,6 +16,7 @@
		is_python_3 = (sys.version_info.major == 3)
		is_windows = (os.name == 'nt')

Testflows -> Pytest #454

Testflows -> Pytest #454

Conversation

slvrtrn commented Sep 13, 2024 • edited Loading

alesapin commented Sep 16, 2024

Choose a reason for hiding this comment

slvrtrn Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slvrtrn Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slvrtrn Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slvrtrn commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slvrtrn commented Sep 19, 2024

alesapin left a comment

Choose a reason for hiding this comment

slvrtrn commented Sep 13, 2024 •

edited

Loading

slvrtrn Sep 17, 2024 •

edited

Loading

slvrtrn Sep 17, 2024 •

edited

Loading

slvrtrn Sep 17, 2024 •

edited

Loading