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

TST: add end-to-end tests with RE/db #8

Closed
wants to merge 5 commits into from
Closed

Conversation

mrakitin
Copy link
Contributor

A follow-up PR to #6 to complement the test suite with the integration tests using RunEngine and databroker instances.

I've also noticed some linter issues/suggestions when running it locally (the pylint checks are currently disabled in the CI config):

❯ pylint redis_json_dict
************* Module redis_json_dict
src/redis_json_dict/__init__.py:13:0: C0411: first party import "from redis_json_dict.redis_json_dict import ObservableMapping, ObservableSequence, RedisJSONDict" should be placed before "from ._version import version as __version__" (wrong-import-order)
************* Module src.redis_json_dict._version
src/redis_json_dict/_version.py:1:0: C0114: Missing module docstring (missing-module-docstring)
src/redis_json_dict/_version.py:6:4: C0103: Class name "VERSION_TUPLE" doesn't conform to PascalCase naming style (invalid-name)
src/redis_json_dict/_version.py:8:4: C0103: Class name "VERSION_TUPLE" doesn't conform to PascalCase naming style (invalid-name)
src/redis_json_dict/_version.py:15:14: C0103: Constant name "version" doesn't conform to UPPER_CASE naming style (invalid-name)
************* Module src.redis_json_dict.redis_json_dict
src/redis_json_dict/redis_json_dict.py:1:0: C0114: Missing module docstring (missing-module-docstring)
src/redis_json_dict/redis_json_dict.py:46:16: E1101: Module 'orjson' has no 'loads' member (no-member)
src/redis_json_dict/redis_json_dict.py:59:15: E1101: Module 'orjson' has no 'dumps' member (no-member)
src/redis_json_dict/redis_json_dict.py:72:4: W0221: Number of parameters was 1 in 'MutableMapping.update' and is now 2 in overriding 'RedisJSONDict.update' method (arguments-differ)
src/redis_json_dict/redis_json_dict.py:72:4: W0221: Variadics removed in overriding 'RedisJSONDict.update' method (arguments-differ)
src/redis_json_dict/redis_json_dict.py:76:19: E1101: Module 'orjson' has no 'dumps' member (no-member)
src/redis_json_dict/redis_json_dict.py:87:0: C0115: Missing class docstring (missing-class-docstring)
src/redis_json_dict/redis_json_dict.py:122:0: C0115: Missing class docstring (missing-class-docstring)
src/redis_json_dict/redis_json_dict.py:152:15: E1120: No value for argument 'on_changed' in constructor call (no-value-for-parameter)
src/redis_json_dict/redis_json_dict.py:183:15: W0212: Access to a protected member _mapping of a client class (protected-access)
src/redis_json_dict/redis_json_dict.py:185:15: W0212: Access to a protected member _sequence of a client class (protected-access)

-----------------------------------
Your code has been rated at 7.42/10

@danielballan
Copy link
Contributor

danielballan commented Feb 17, 2024

I would rather not weigh down this lightweight and general-purpose package with tests for a specific use case that has a lot of dependencies. Perhaps a more targeted test can do what we need here: for example, a test that the built-in json module can serialize a deep copy of a nested RedisJSONDict. If that works, then Bluesky, Databroker, Kafka, and other applications will be happy.

@mrakitin
Copy link
Contributor Author

It's fair. On the other hand, those tests can (1) uncover unforeseen integration/dependencies issues with the bluesky stack, and (2) demonstrate how to hook it to another software, in this case, bluesky. We don't inject any bluesky dependencies into the requirements, only optional.

We have to have those tests somewhere - probably profile-collection-ci is a better place for that.

act -W .github/workflows/ci.yml -j checks --matrix python-version:3.11
@mrakitin
Copy link
Contributor Author

mrakitin commented Oct 4, 2024

Closing. I has to live elsewhere.

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