Set up github actions + remove databricks-specific test infra #3

susodapop · 2022-06-07T20:47:32Z

Description

Workflows

This PR introduces github workflows to perform unit tests, linting, and static type analysis for every pull request on this repository. You can run these locally with the following commands:

poetry run pytest tests
poetry run black --check src
poetry run mypy src

The workflows are copied directly from the https://github.com/databricks/databricks-sql-cli repository.

Cleanup Databricks specific test infra

This repository contains resources that Databricks uses for integration tests. But the tests themselves were never housed in this directory. I've cleaned out these files and references from this repository for cleanliness. Because without the actual integration tests / infrastructure, these files serve no purpose within this repository.

Will need to follow-up with the team about how and where to house the integration tests going forward.

How is this tested?

The checks will automatically appear at the bottom of this PR. If they pass, then black,mypy, and pytest` all exited with code 0.

susodapop · 2022-06-07T21:00:33Z

python-37-slim-buster.dockerfile

@@ -1 +0,0 @@
-FROM python:3.7-slim-buster


This file is not used anywhere in this repository. Rather it is referenced from outside the repository.

susodapop · 2022-06-07T21:00:42Z

test-container-with-reqs.dockerfile

@@ -1,3 +0,0 @@
-RUN pip install -r dev_requirements.txt


Ditto above.

susodapop · 2022-06-07T21:00:52Z

tests/docker-run-unit.sh

@@ -1,5 +0,0 @@
-#!/bin/bash


Ditto above.

susodapop · 2022-06-07T21:02:02Z

tests/test_fetches_bench.py

@@ -39,6 +40,7 @@ def make_dummy_result_set_from_initial_results(arrow_table):
                          for col_id in range(arrow_table.num_columns)]
        return rs

+    @pytest.mark.skip(reason="Test has not been updated for latest connector API (June 2022)")


This test fails because this test file was not updated as the connector API changed. Unclear if we should just remove this test completely or update it. I didn't find any notes on this subject.

@susodapop was there any breaking changes in the public API of the PySQL?

Did we do breaking change as part of open sourcing? or the breaking change was introduced before that?

From what I recall the test is broken because we changed the API and didn't update this test accordingly. Since I'm not familiar with what this test is meant to do or why it wasn't updated I don't have further context.

susodapop · 2022-06-07T21:02:44Z

thrift_dbr_test/thrift_basic_tests.py

@@ -1,20 +0,0 @@
-import databricks.sql


These test files are intended to be called in a bazel test run which is not defined in this repository.

moderakh · 2022-06-16T16:40:52Z

tests/test_fetches_bench.py

@@ -39,6 +40,7 @@ def make_dummy_result_set_from_initial_results(arrow_table):
                          for col_id in range(arrow_table.num_columns)]
        return rs

+    @pytest.mark.skip(reason="Test has not been updated for latest connector API (June 2022)")


@susodapop was there any breaking changes in the public API of the PySQL?

Did we do breaking change as part of open sourcing? or the breaking change was introduced before that?

moderakh · 2022-06-16T16:44:43Z

.github/workflows/code-quality-checks.yml

+      - name: Install library
+        run: poetry install --no-interaction
+      #----------------------------------------------
+      #              run test suite


@susodapop
which of the following is true:

does this install the connector package (same way as installed by end user) and runs the tests against the package?

runs the tests against a source folder of the connector package without installing it as a package.

let's say we have a bug in packaging of the connector, that bug will be discovered if we do 1) but not if we do 2). I wonder which approach we are doing?

Actually it's a combination of both. Since we are now using poetry, it guarantees that when we runpoetry install, the local code will be packaged and then installed equivalent to a user installing the published .tar.gz file with pip or similar.

I believe we're safe on this front.

moderakh · 2022-06-16T16:46:17Z

.github/workflows/code-quality-checks.yml

+        id: setup-python
+        uses: actions/setup-python@v2
+        with:
+          python-version: 3.7


do we need to extend the test matrix to cover other supported versions?
3.8 or 3.9?

which versions do we officially support?

It's easy enough to duplicate this workflow for Python 3.8 through 3.10.

I think to minimise duplication we can have one workflow that uses the latest version of python and another that uses the oldest version we support.

susodapop · 2022-06-24T17:47:21Z

I'm merging these changes now. Will file a follow-up PR with the cross version Python unit tests.

catalog in get_table_names adjusted with backquotes

* move py.typed to correct places https://peps.python.org/pep-0561/ says 'For namespace packages (see PEP 420), the py.typed file should be in the submodules of the namespace, to avoid conflicts and for clarity.'. Previously, when I added the py.typed file to this project, #382 , I was unaware this was a namespace package (although, curiously, it seems I had done it right initially and then changed to the wrong way). As PEP 561 warns us, this does create conflicts; other libraries in the databricks namespace package (such as, in my case, databricks-vectorsearch) are then treated as though they are typed, which they are not. This commit moves the py.typed file to the correct places, the submodule folders, fixing that problem. Signed-off-by: wyattscarpenter <wyattscarpenter@gmail.com> * change target of mypy to src/databricks instead of src. I think this might fix the CI code-quality checks failure, but unfortunately I can't replicate that failure locally and the error message is unhelpful Signed-off-by: wyattscarpenter <wyattscarpenter@gmail.com> * Possible workaround for bad error message 'error: --install-types failed (no mypy cache directory)'; see python/mypy#10768 (comment) Signed-off-by: wyattscarpenter <wyattscarpenter@gmail.com> * fix invalid yaml syntax Signed-off-by: wyattscarpenter <wyattscarpenter@gmail.com> * Best fix (#3) Fixes the problem by cding and supplying a flag to mypy (that mypy needs this flag is seemingly fixed/changed in later versions of mypy; but that's another pr altogether...). Also fixes a type error that was somehow in the arguments of the program (?!) (I guess this is because you guys are still using implicit optional) --------- Signed-off-by: wyattscarpenter <wyattscarpenter@gmail.com> * return the old result_links default (#5) Return the old result_links default, make the type optional, & I'm pretty sure the original problem is that add_file_links can't take a None, so these statements should be in the body of the if-statement that ensures it is not None Signed-off-by: wyattscarpenter <wyattscarpenter@gmail.com> * Update src/databricks/sql/utils.py "self.download_manager is unconditionally used later, so must be created. Looks this part of code is totally not covered with tests 🤔" Co-authored-by: Levko Kravets <levko.ne@gmail.com> Signed-off-by: wyattscarpenter <wyattscarpenter@gmail.com> --------- Signed-off-by: wyattscarpenter <wyattscarpenter@gmail.com> Co-authored-by: Levko Kravets <levko.ne@gmail.com>

susodapop added 4 commits June 7, 2022 15:43

Remove databricks-specific test fixtures

b83a2cb

Introduce pytest, black, and mypy workflows

14e8229

Skip test_fetches_bench case

0226b61

Remove references to databricks-specific testing infra

0bd92fa

susodapop commented Jun 7, 2022

View reviewed changes

susodapop requested review from sander-goos, arikfr and moderakh June 7, 2022 21:02

arikfr approved these changes Jun 16, 2022

View reviewed changes

moderakh reviewed Jun 16, 2022

View reviewed changes

susodapop merged commit 1b866f0 into main Jun 24, 2022

susodapop deleted the neil-degrasse-tyson branch June 24, 2022 17:47

PeterDKay referenced this pull request in sede-open/databricks-sql-python May 25, 2023

Merge pull request #3 from PeterDKay/task/alter-get-tables-for-UC

3f211ec

catalog in get_table_names adjusted with backquotes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up github actions + remove databricks-specific test infra #3

Set up github actions + remove databricks-specific test infra #3

susodapop commented Jun 7, 2022 •

edited

Loading

susodapop Jun 7, 2022

susodapop Jun 7, 2022

susodapop Jun 7, 2022

susodapop Jun 7, 2022

moderakh Jun 16, 2022

susodapop Jun 16, 2022

susodapop Jun 7, 2022

moderakh Jun 16, 2022

moderakh Jun 16, 2022

susodapop Jun 16, 2022

moderakh Jun 16, 2022

susodapop Jun 16, 2022

susodapop commented Jun 24, 2022

Set up github actions + remove databricks-specific test infra #3

Set up github actions + remove databricks-specific test infra #3

Conversation

susodapop commented Jun 7, 2022 • edited Loading

Description

Workflows

Cleanup Databricks specific test infra

How is this tested?

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

susodapop commented Jun 24, 2022

susodapop commented Jun 7, 2022 •

edited

Loading