Add tests for caching.

[dedan]

always import modules instead of classes

and move TestExtractor into its own module to be re-used for other tests

+@pcorpet

---

This change is

[dedan]

+@pcorpet

---

Review status: 0 of 6 files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[pcorpet]

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

tests/collection_test.py, line 11 [r1] (raw file):
I suggest we use the mock module: https://docs.python.org/3/library/unittest.mock.html instead.

---

tests/collection_test.py, line 40 [r1] (raw file):
this function is very hard to read: please add blank lines and comment for each block so we can follow what's happening.

---

tests/collection_test.py, line 48 [r1] (raw file):
use another variable name so that it's easier to understand that it's completely different

collection2 = ...

---

tests/collection_test.py, line 52 [r1] (raw file):
Nit: here you're are doing 3 tests.

1. You are checking that when you use collection.run the text_extractor.extract is called exactly once.
2. You check that creating another collection doesn't call the extract method again.
3. You check that changing the source code and changing the collection calls the extract method again.

What I suggest to reduce that:

• Don't assertEquals on the number of calls for 1. Instead just store the number (so that if in the future your code calls it twice or three times extract, this test won't fail)
• In 2, instead of checking it's 1, just check that it's the same number as above (i.e. that there are no new calls).
• opt - move 3 to another test, and only check that is greater than before (not necessary exactly 2).

---

tests/collection_test.py, line 54 [r1] (raw file):
modifying inner properties of an object is usually considered as bad testing practice (as the test need to be updated if you change how the object works internally).

Can you try using reflection to somehow actually modify the source code?

---

Comments from the review on Reviewable.io

[dedan]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

tests/collection_test.py, line 11 [r1] (raw file):
I cannot use the mock library because it does not work with pickling. I mention this in the docstring, but that's probably not the most visible place. I will add a comment right above the class definition

---

tests/collection_test.py, line 40 [r1] (raw file):
Done.

---

tests/collection_test.py, line 48 [r1] (raw file):
Done.

---

tests/collection_test.py, line 52 [r1] (raw file):
Great advice. Testing is really not my strength.

I reduced it to testing only 1 and 2 for now, because that's also what the name of the function says

---

tests/collection_test.py, line 54 [r1] (raw file):
argh, I already had a strong feeling that that's probably not allowed.

I will remember it when I will write a test for the changed hash, but I removed this case for now anyways (see above)

---

Comments from the review on Reviewable.io

[pcorpet]

---

Reviewed 3 of 3 files at r2, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

tests/collection_test.py, line 11 [r1] (raw file):
got it. Please make this class protected to this module (prefix by _)

---

tests/collection_test.py, line 53 [r4] (raw file):
put the expected first:

assertEqual(counter_first_run, test_extractor...counter)

so that if there's a failure the error message will show it correctly.

---

Comments from the review on Reviewable.io

[dedan]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

tests/collection_test.py, line 11 [r1] (raw file):
Done.

---

tests/collection_test.py, line 53 [r4] (raw file):
Done.

---

Comments from the review on Reviewable.io