Repository: glotzerlab/signac
Pull request #676 (2022-02-04 04:58:59)

Improve test coverage of Collection class's find methods.
## Description
While working on removing internal usage of the `Collection` class in #667 for signac 2.0, I wanted to improve the test coverage for the `Collection` class's find methods to ensure that the new code in signac 2.0 does not introduce bugs/regressions.

I fixed one bug when searching by the primary key (an `AssertionError` was raised unexpectedly when searching for an id that did not exist, when the result should just be an empty set). Additionally, I expanded test coverage of several other features relating to search operators like `$and` and `$type`. I also fixed some mistakes/omissions in the class documentation.

## Motivation and Context
These tests will be used to improve #667 and ensure that we have greater coverage of our `find` API in the smaller Collection-like class in that PR.

## Types of Changes
- [ ] Documentation update
- [x] Bug fix
- [ ] New feature
- [ ] Breaking change<sup>1</sup>

<sup>1</sup>The change breaks (or has the potential to break) existing functionality.

## Checklist:
- [x] I am familiar with the [**Contributing Guidelines**](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md).
- [x] I agree with the terms of the [**Contributor Agreement**](https://github.com/glotzerlab/signac/blob/master/ContributorAgreement.md).
- [x] My name is on the [list of contributors](https://github.com/glotzerlab/signac/blob/master/contributors.yaml).
- [x] My code follows the [code style guideline](https://github.com/glotzerlab/signac/blob/master/CONTRIBUTING.md#code-style) of this project.
- [x] The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:
- [ ] I have updated the API documentation as part of the package doc-strings.
- [ ] I have created a separate pull request to update the [framework documentation](https://docs.signac.io/) on [signac-docs](https://github.com/glotzerlab/signac-docs) and linked it here.
- [ ] I have updated the [changelog](https://github.com/glotzerlab/signac/blob/master/changelog.txt) and added all related issue and pull request numbers for future reference (if applicable). See example below.

codecov[bot]: # [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/676?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) Report
> Merging [#676](https://codecov.io/gh/glotzerlab/signac/pull/676?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (012daf0) into [master](https://codecov.io/gh/glotzerlab/signac/commit/6603bc40616f560065a9b5f1194cd0d226a7ba38?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) (6603bc4) will **increase** coverage by `0.14%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/glotzerlab/signac/pull/676/graphs/tree.svg?width=650&height=150&src=pr&token=FghbjTyzgM&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab)](https://codecov.io/gh/glotzerlab/signac/pull/676?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab)

```diff
@@            Coverage Diff             @@
##           master     #676      +/-   ##
==========================================
+ Coverage   78.42%   78.56%   +0.14%     
==========================================
  Files          65       65              
  Lines        7129     7129              
  Branches     1564     1564              
==========================================
+ Hits         5591     5601      +10     
+ Misses       1230     1225       -5     
+ Partials      308      303       -5     
```


| [Impacted Files](https://codecov.io/gh/glotzerlab/signac/pull/676?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab) | Coverage Δ | |
|---|---|---|
| [signac/contrib/collection.py](https://codecov.io/gh/glotzerlab/signac/pull/676/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab#diff-c2lnbmFjL2NvbnRyaWIvY29sbGVjdGlvbi5weQ==) | `85.99% <100.00%> (+1.97%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/glotzerlab/signac/pull/676?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/glotzerlab/signac/pull/676?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab). Last update [6603bc4...012daf0](https://codecov.io/gh/glotzerlab/signac/pull/676?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glotzerlab).


Patch:
diff --git a/changelog.txt b/changelog.txt
index fca60e2d..8897b1e9 100644
--- a/changelog.txt
+++ b/changelog.txt
@@ -35,6 +35,7 @@ Fixed
 
  - ``H5Store.mode`` returns the file mode (#607).
  - User-provided path functions now raise an error if not unique (#666).
+ - ``Collection`` class no longer raises an error when searching by a primary key that does not exist (#676).
 
 [1.7.0] -- 2021-06-08
 ---------------------
diff --git a/signac/contrib/collection.py b/signac/contrib/collection.py
index 7ddf09ed..c27ffa09 100644
--- a/signac/contrib/collection.py
+++ b/signac/contrib/collection.py
@@ -796,7 +796,8 @@ class Collection:
             When Bad operator expression/ Bad operator placement or
             the expression-operator is unknown.
         ValueError
-            The value is not bool when operator for '$exists' operator.
+            The value is not bool for '$exists' operator or not a
+            supported type for '$type' operator.
 
         """
         logger.debug(f"Find documents for expression '{key}: {value}'.")
@@ -876,8 +877,8 @@ class Collection:
         # Check if filter contains primary key, in which case we can
         # immediately reduce the result.
         _id = expr.pop(self._primary_key, None)
-        if _id is not None and _id in self:
-            reduce_results({_id})
+        if _id is not None:
+            reduce_results({_id} if _id in self else set())
 
         # Extract all logical-operator expressions for now.
         or_expressions = expr.pop("$or", None)
@@ -1049,7 +1050,7 @@ class Collection:
 
                     .. code-block:: python
 
-                        project.find({"protocol": {"$type": str}})
+                        project.find({"protocol": {"$type": "str"}})
 
             Finds all docs, where the value of protocol is of type str.
             Other types that can be checked are: *int*, *float*, *bool*, *list*, and *null*.


Test patch:
diff --git a/tests/test_collection.py b/tests/test_collection.py
index 3af28b8b..604f2a44 100644
--- a/tests/test_collection.py
+++ b/tests/test_collection.py
@@ -37,6 +37,8 @@ ARRAY_EXPRESSIONS = [
 
 LOGICAL_EXPRESSIONS = [
     ({"$and": []}, ValueError),
+    ({"$and": {}}, ValueError),
+    ({"$and": ""}, ValueError),
     ({"a": {"$and": []}}, KeyError),
     ({"a": {"$and": [{"b": 0}]}}, KeyError),
     ({"$and": [{"a": n}]}, 1),
@@ -51,6 +53,14 @@ LOGICAL_EXPRESSIONS = [
     ({"$or": [{"a": n}, {"$not": {"a": n}}]}, N),
 ]
 
+INVALID_SYNTAX_EXPRESSIONS = [
+    ({"a": {"$$lt": N}}, KeyError),  # Bad operator expression
+    ({"a": {"lt$": N}}, KeyError),  # Bad operator placement
+    ({"a": {"$exists": N}}, ValueError),  # Value of $exists must be boolean
+    ({"a": {"$nonexistent": N}}, KeyError),  # Unknown expression-operator
+    ({"a": {"$type": N}}, ValueError),  # Invalid $type argument
+]
+
 
 class TestCollection:
     @pytest.fixture(autouse=True)
@@ -247,6 +257,14 @@ class TestCollection:
         assert len(self.c.find()) == len(docs)
         assert {doc["a"] for doc in docs} == {doc["a"] for doc in self.c.find()}
 
+    def test_find_id(self):
+        assert len(self.c.find()) == 0
+        assert len(self.c.find({"_id": "0"})) == 0
+        docs = [{"a": i, "_id": str(i)} for i in range(10)]
+        self.c.update(docs)
+        assert len(self.c.find()) == len(docs)
+        assert len(self.c.find({"_id": "0"})) == 1
+
     def test_find_integer(self):
         assert len(self.c.find()) == 0
         assert list(self.c.find()) == []
@@ -587,6 +605,13 @@ class TestCollection:
                 assert len(self.c.find({"$not": expr})) == N - expectation
                 assert len(self.c.find({"$not": {"$not": expr}})) == expectation
 
+    def test_find_invalid_syntax(self):
+        self.c.update(ARITHMETIC_DOCS)
+        assert len(self.c) == len(ARITHMETIC_DOCS)
+        for expr, expectation in INVALID_SYNTAX_EXPRESSIONS:
+            with pytest.raises(expectation):
+                self.c.find(expr)
+
 
 class TestCompressedCollection(TestCollection):
     @pytest.fixture(autouse=True)