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.

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

PLAT-107: Clean up #1141

Merged
merged 34 commits into from
Jan 2, 2024
Merged

PLAT-107: Clean up #1141

merged 34 commits into from
Jan 2, 2024

Conversation

ethho
Copy link
Contributor

@ethho ethho commented Dec 15, 2023

Clean up pytest suite to keep patterns consistent. For instance, aggregate commonly used fixtures in conftest.py

  • Clean test_adapted_attributes
  • Clean test_admin
  • Clean test_aggr_regressions
  • Clean test_alter
  • Clean test_attach
  • Fix ImportError
  • Make stores config serializable
  • Correct use of tmpdir_factory
  • Clean up test_autopopulate
  • Clean up other modules

import datajoint as dj
import inspect


Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this schema to a separate module per @A-Baji 's previous suggestion.


@pytest.fixture
def schema_name(prefix):
return prefix + "_test_custom_datatype"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched this to a fixture, because it uses prefix which is now also a fixture.

self.insert1(dict(key, crop_image=dict()))

Crop.populate()
from . import schema
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved from methods to top-level functions.

@@ -207,6 +208,8 @@ def test_insert_longblob(schema_any):
dtype=[("hits", "O"), ("sides", "O"), ("tasks", "O"), ("stage", "O")],
),
}
assert fetched['id'] == expected['id']
assert np.array_equal(fetched['data'], expected['data'])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Effectively the same logic, but resolves a DeprecationWarning.

yield Subjects
Subjects.drop()


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved Schema as a module level definition. This keeps it consistent with other schemas in the test suite.

@@ -12,36 +11,41 @@ class NanTest(dj.Manual):
"""


@pytest.fixture(scope="module")
def schema(connection_test):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to give these fixtures more descriptive and unique names. While reusing the name schema between test modules technically works, unique names like schema_nan make it clearer that tests in different modules are using entirely different fixtures.

def setup_class(request, schema):
@pytest.fixture
def arr_a():
return np.array([0, 1 / 3, np.nan, np.pi, np.nan])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This array is used in multiple tests, so I moved to a fixture in the spirit of DRY.

def trash(schema_any):
return schema.UberTrash()


Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to conftest.py

cls.ephys = Ephys()
cls.channel = Ephys.Channel()
cls.img = Image()
cls.trash = UberTrash()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of setup_class, these tests use fixtures like subject and experiment which are now defined in conftest.py.

)
from . import PREFIX, CONN_INFO
from .schema_simple import *
from .schema import *
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More succinct, relatively safe (since we define __all__ in the schema* modules, and consistent with how we import table definitions elsewhere.

@ethho ethho marked this pull request as ready for review December 18, 2023 21:53
@ethho ethho requested a review from A-Baji December 18, 2023 21:53
Ensures that dj.blob.use_32bit_dims is turned off
even if test_insert_longblob fails.
@ethho
Copy link
Contributor Author

ethho commented Dec 20, 2023

CI tests that use minio_client failed due to expired certs for docker://datajoint/nginx:v0.2.7. Created ticket for updating certs and image: https://datajoint.atlassian.net/browse/DEV-397?atlOrigin=eyJpIjoiZDQzNDJkNDNmZGRhNGI2NWIzMDZlM2Y4MDM3ZmI1MTciLCJwIjoiaiJ9

@ethho ethho marked this pull request as draft December 20, 2023 00:51
@ethho ethho marked this pull request as ready for review December 20, 2023 01:48
@A-Baji A-Baji merged commit 88c96e6 into datajoint:dev-tests Jan 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants