Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make tests configurable against backend #2351

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Make tests configurable against backend #2351

merged 1 commit into from
Jul 30, 2024

Conversation

jieguangzhou
Copy link
Collaborator

Description

Related Issues

Checklist

  • Is this code covered by new or existing unit tests or integration tests?
  • Did you run make unit_testing and make integration-testing successfully?
  • Do new classes, functions, methods and parameters all have docstrings?
  • Were existing docstrings updated, if necessary?
  • Was external documentation updated, if necessary?

Additional Notes or Comments

@jieguangzhou jieguangzhou marked this pull request as ready for review July 30, 2024 08:02
@jieguangzhou jieguangzhou linked an issue Jul 30, 2024 that may be closed by this pull request
[SIMPLIFY-2] Make tests configurable against backend #2315
Closed
4 tasks
jieguangzhou
jieguangzhou commented Jul 30, 2024
View reviewed changes
test/configs/default.yaml Outdated
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will only need this in the future when we have completed separating the plugins.

jieguangzhou
jieguangzhou commented Jul 30, 2024
View reviewed changes
test/unittest/backends/base/test_query.py


@pytest.mark.parametrize("db", [DBConfig.mongodb_empty], indirect=True)
def test_execute_complex_query_mongodb(db):
Copy link
Collaborator Author

@jieguangzhou jieguangzhou Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to test.backends.mongodb.test_query.py

jieguangzhou
jieguangzhou commented Jul 30, 2024
View reviewed changes
test/unittest/backends/base/test_query.py


@pytest.mark.parametrize("db", [DBConfig.sqldb_empty], indirect=True)
def test_execute_complex_query_sqldb_auto_schema(db):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to test.backends.ibis.test_query.py

jieguangzhou
jieguangzhou commented Jul 30, 2024
View reviewed changes
test/unittest/backends/ibis/__init__.py
Comment on lines +1 to +8
import os

import pytest

skip = not os.environ.get('SUPERDUPER_CONFIG', "").endswith('ibis.yaml')

if skip:
pytest.skip("Skipping this file for now", allow_module_level=True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move the whole ibis test module to the ibis plugins next.

jieguangzhou
jieguangzhou commented Jul 30, 2024
View reviewed changes
test/unittest/backends/mongodb/__init__.py
Comment on lines +1 to +8
import os

import pytest

skip = not os.environ.get('SUPERDUPER_CONFIG', "").endswith('mongodb.yaml')

if skip:
pytest.skip("Skipping this file for now", allow_module_level=True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move the whole ibis test module to the mongodb plugins next.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

kartik4949
kartik4949 reviewed Jul 30, 2024
View reviewed changes
test/unittest/jobs/test_task_workflow.py Outdated
@@ -55,7 +50,7 @@ def test_downstream_task_workflows_are_triggered(db, data, flatten):

downstream_listener = downstream_model.to_listener(
key=upstream_listener.outputs_key,
select=upstream_listener.outputs_select,
select=db[upstream_listener.outputs].select(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to discuss this

Copy link
Collaborator Author

@jieguangzhou jieguangzhou Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted to upstream_listener.outputs_select

@jieguangzhou jieguangzhou requested a review from kartik4949 July 30, 2024 13:01
blythed
blythed reviewed Jul 30, 2024
View reviewed changes
Makefile
@@ -225,4 +227,6 @@ ext_testing: ## Execute integration testing


usecase_testing: ## Execute usecase testing
pytest $(PYTEST_ARGUMENTS) ./test/integration/usecase
# TODO After we have completed separating the plugins, we can run the tests only on default.yaml.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should somehow sync with the use-cases which @kartik4949 has added.

blythed
blythed reviewed Jul 30, 2024
View reviewed changes
test/integration/conftest.py
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably these integration tests will go to plugin tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some need to be moved, but some integration tests that use the default configuration must be retained to test complex workflows.​

blythed
blythed reviewed Jul 30, 2024
View reviewed changes
test/unittest/base/test_datalayer.py
def test_insert_sql_db(db):
db.cfg.auto_schema = True
Copy link
Collaborator

@blythed blythed Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should think about making auto_schema true by default in tests also (for later).

blythed
blythed reviewed Jul 30, 2024
View reviewed changes
test/integration/usecase/test_eager.py
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is eager mode a use-case? Not critical currently, but might be worth reorganizing a bit.

blythed
blythed approved these changes Jul 30, 2024
View reviewed changes
Copy link
Collaborator

@blythed blythed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice PR! That's a lot of work done to get this logic uniform. Thanks for the hard work.

@blythed
Copy link
Collaborator

blythed commented Jul 30, 2024

Very nice PR! That's a lot of work done to get this logic uniform. Thanks for the hard work.

Interestingly, there wasn't much to change in the main codebase, only the tests.

@blythed blythed merged commit 2aafee9 into superduper-io:main Jul 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blythed blythed blythed approved these changes

@kartik4949 kartik4949 Awaiting requested review from kartik4949

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[SIMPLIFY-2] Make tests configurable against backend
3 participants
@jieguangzhou @blythed @kartik4949