aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
436 stars 191 forks source link

Re-Implement `has_key` Filter Operator for SQLite Backend #6606

Closed rabbull closed 6 days ago

rabbull commented 1 week ago

This PR addresses the issues described in #6552 and uncovered in #6256. The has_key operator implementation for the SQLite backend was previously removed in #6497 due to problematic behavior and the potential for silent failures.

In this PR, has_key is reintroduced by adopting @giovannipizzi's implementation from #6552. Additionally, unit tests for JSON filters have been re-enabled and extended. The new tests are able to identify failures in the original implementation and pass with the current one.

codecov[bot] commented 1 week ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 77.90%. Comparing base (ef60b66) to head (7d3a6e3). Report is 134 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6606 +/- ## ========================================== + Coverage 77.51% 77.90% +0.40% ========================================== Files 560 567 +7 Lines 41444 42147 +703 ========================================== + Hits 32120 32831 +711 + Misses 9324 9316 -8 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

GeigerJ2 commented 1 week ago

Before I add my formal review, just a note to remove the @pytest.mark.requires_psql that were added in #6497, as these tests should now be passing again.

rabbull commented 1 week ago

Before I add my formal review, just a note to remove the @pytest.mark.requires_psql that were added in #6497, as these tests should now be passing again.

@GeigerJ2: All tests disabled in #6497 are now re-enabled in the latest commit, and are all passing.

It’s also worth mentioning that the re-enabled tests are run with -m 'not requires_psql' to specifically use the SQLite backend. I'm uncertain if this is the best approach or if it could cause any unintended side effects, so I’m sharing my method here for anyone with prior knowledge to review and point out any potential issues.

GeigerJ2 commented 1 week ago

Great, thanks @rabbull! Providing some background info on your question: By default, non-marked tests, when being run locally, use the psql_dos storage backend. This behavior can be changed via pytest -m 'not requires_psql', as you said, or pytest -m presto, which is basically a shortcut for not requires_psql and not requires_rmq (the verdi presto command was recently added to simplify the setup of a profile without the PSQL and RMQ services).

When testing via GitHub actions, all non-tagged tests (more specifically, not marked with requires_rmq and requires_psql) are also automatically being run with the -m presto flag, see here. This is the ci-code/tests-presto check. So, when running via GH CI, both storage backends are used, while locally, you used the right option to test for the correct behavior when using SQLite.

danielhollas commented 5 days ago

Hi @GeigerJ2 @rabbull, thanks for for making the SQLite backend better!

As a person that implemented the presto flag to pytest, I do agree that the current way of selecting the SQLite backend in tests implicitly via the pytest markers is very clunky. I believe we should have a dedicated option for selecting the backend (and specifying SQLite would then automatically set the "not requires_pgsql").