Convert unittest class base tests to pytest function tests #553

rootart · 2021-10-09T13:18:46Z

Many thanks for the great package folks 🤗!
I managed to convert the majority of tests from unittest class base tests to pytest (related issue #521 )and will appreciate your feedback and reviews.
SessionTests tests were not migrated as they look to be very much coupled with ones from Django.

Fixes #521

codecov · 2021-10-09T14:41:35Z

Codecov Report

Merging #553 (24f6c8a) into master (6ff7c54) will decrease coverage by 26.0%.
The diff coverage is 14.7%.

@@            Coverage Diff            @@
##           master    #553      +/-   ##
=========================================
- Coverage    83.8%   57.8%   -25.9%     
=========================================
  Files          21      39      +18     
  Lines        1175    2464    +1289     
  Branches       60      62       +2     
=========================================
+ Hits          984    1424     +440     
- Misses        185    1032     +847     
- Partials        6       8       +2

Flag	Coverage Δ
mypy	`34.0% <14.7%> (+0.3%)`	⬆️
tests	`83.1% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tests/test_session.py	`6.8% <6.8%> (ø)`
tests/test_backend.py	`14.2% <8.7%> (ø)`
tests/test_client.py	`30.1% <30.1%> (ø)`
tests/test_cache_options.py	`33.8% <33.8%> (ø)`
tests/test_connection_string.py	`66.7% <66.7%> (ø)`
tests/conftest.py	`75.0% <75.0%> (ø)`
tests/settings/sqlite.py	`100.0% <100.0%> (ø)`
tests/settings/sqlite_lz4.py	`100.0% <0.0%> (ø)`
tests/test_hashring.py	`19.1% <0.0%> (ø)`
tests/settings/sqlite_sentinel.py	`100.0% <0.0%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ff7c54...24f6c8a. Read the comment docs.

WisdomPill

Great job and thank you for contributing. We were talking in the past too separate classes in different files, could you separate tests that were in the same class in different files?

I did not review everything because it seems that the same suggestions will be made for the rest of the file and I will need anyway to review everything after the splitting and small fixes will be made

WisdomPill · 2021-10-09T14:42:31Z

tests/test_backend.py

-        res = self.cache.set("test_key_nx", 1, nx=True)
-        self.assertTrue(res)
+        res = default_cache.set("test_key_nx", 1, nx=True)
+        assert res


I prefer it to be more specific

Suggested change

assert res

assert res == True

WisdomPill · 2021-10-09T14:44:58Z

tests/test_backend.py

+
+
+@pytest.fixture()
+def available_caches(settings):


could you add a little bit of type hints? and maybe also separate them? just so that it looks cleaner and you do not have to do the unpacking in every test

WisdomPill · 2021-10-09T14:47:40Z

tests/test_backend.py

-        res = self.cache.delete_pattern("*foo-a*")
-        self.assertTrue(bool(res))
+    res = cache.delete_pattern("*foo-a*")
+    assert bool(res)


better to check if it is True

Suggested change

assert bool(res)

assert res is True

WisdomPill · 2021-10-09T14:48:29Z

tests/test_backend.py

-        self.assertFalse(res)
+        default_cache.set("test_key_nx", 1)
+        res = default_cache.set("test_key_nx", 2, timeout=2, nx=True)
+        assert not res


Suggested change

assert not res

assert res is False

WisdomPill · 2021-10-09T14:51:48Z

tests/test_backend.py

-        res = self.cache.delete("a")
-        self.assertFalse(bool(res))
+        res = default_cache.delete("a")
+        assert not bool(res)


Suggested change

assert not bool(res)

assert res is False

WisdomPill · 2021-10-09T14:52:56Z

tests/test_backend.py

+        assert res
+        res = default_cache.delete("b")
+        assert type(res) == bool
+        assert not res


Suggested change

assert not res

assert res is False

WisdomPill · 2021-10-09T14:54:10Z

tests/test_backend.py

+        mocker.patch("django_redis.cache.DJANGO_VERSION", new=(3, 1, 0, "final", 0))
+        default_cache.set("a", 1)
+        res = default_cache.delete("a")
+        assert type(res) == bool


let's keep isinstance for simplicity and consistency

Suggested change

assert type(res) == bool

assert isinstance(res, bool)

Agree, I've been using https://github.com/pytest-dev/unittest2pytest to convert larger test classes and didn't thoroughly check results.

WisdomPill · 2021-10-09T14:54:51Z

tests/test_backend.py

+        assert type(res) == bool
+        assert res
+        res = default_cache.delete("b")
+        assert type(res) == bool


let's keep isinstance for simplicity and consistency

Suggested change

assert type(res) == bool

assert isinstance(res, bool)

WisdomPill · 2021-10-09T14:55:41Z

tests/test_backend.py

+    def test_delete_many(self, default_cache):
+        default_cache.set_many({"a": 1, "b": 2, "c": 3})
+        res = default_cache.delete_many(["a", "b"])
+        assert bool(res)


Suggested change

assert bool(res)

assert res is True

Deletion of the multiple keys returns back the number of deleted objects and therefore I used assert bool(res) is True to cover these cases.

WisdomPill · 2021-10-09T14:55:56Z

tests/test_backend.py

+        assert res == {"c": 3}
+
+        res = default_cache.delete_many(["a", "b"])
+        assert not bool(res)


Suggested change

assert not bool(res)

assert res is False

rootart · 2021-10-09T20:45:28Z

Hi @WisdomPill and thank you for the review.

Great job and thank you for contributing. We were talking in the past too separate classes in different files, could you separate tests that were in the same class in different files?

Sure, I will work on it.

I did not review everything because it seems that the same suggestions will be made for the rest of the file and I will need anyway to review everything after the splitting and small fixes will be made

That totally makes sense and I also noticed that a lot of issues were related to the automatic unittest -> pytest migration tool that I used and I wasn't very careful reviewing results 😅

WisdomPill · 2021-10-10T19:14:36Z

It will take me some time to review the whole PR because during the week I do not have much time.
Meanwhile I see that codecov is looking for coverage in tests/ directory, which is not good so maybe it should be filtered out.

Last but not least, I want to raise a question, is there a reason to test for type after all?
Because if you have for example "1" and 1 you can just check if they are equal, it will throw an error if the type is different.
So I think that probably to check the type with isinstance is just redundant.

Python 3.9.1 (default, Jan  8 2021, 17:17:43) 
[Clang 12.0.0 (clang-1200.0.32.28)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> "1" == 1
False
>>>

rootart · 2021-10-11T21:04:02Z

I was probably a bit confused regarding your suggestion of adding type annotations to the provided fixtures and added them to all tests :) Updated version removes that checks and I assume should fix issues with codecov.

Last but not least, I want to raise a question, is there a reason to test for type after all?
No, I don't see many benefits in testing for types and as you fairly pointed most of it will be correctly resolved on the assertion level by the pytest.

rootart · 2021-10-21T19:04:25Z

@WisdomPill would be great if you can approve workflow execution to check tests 🙇

terencehonles · 2021-10-21T23:32:46Z

@WisdomPill would be great if you can approve workflow execution to check tests bow

Done

terencehonles

The change looks good to me, but I guess there's a little bit of a difference of opinion on the asserts 😅

terencehonles · 2021-10-21T23:36:56Z

tests/test_backend.py

-def make_key(key, prefix, version):
-    return f"{prefix}#{version}#{key}"
+@pytest.fixture
+def cache() -> Generator[RedisCache, None, None]:


If you're not supplying the second two type variables this is a shorthand for the same thing

Suggested change

def cache() -> Generator[RedisCache, None, None]:

def cache() -> Iterable[RedisCache]:

terencehonles · 2021-10-21T23:48:34Z

tests/test_backend.py

-        res = self.cache.set("test_key_nx", 1, nx=True)
-        self.assertTrue(res)
+        res = cache.set("test_key_nx", 1, nx=True)
+        assert bool(res) is True


@WisdomPill do you strongly prefer assert bool(res) is True over assert res and assert res is False over assert not res? I find it a bit wordy since it means the same thing, but I understand you already requested the change in #553 (comment)

I prefer assert res is True because it more readable and correct.

From python zen

Python 3.9.1 (default, Jan 8 2021, 17:17:43) [Clang 12.0.0 (clang-1200.0.32.28)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>> import this The Zen of Python, by Tim Peters Beautiful is better than ugly. Explicit is better than implicit. Simple is better than complex. Complex is better than complicated. Flat is better than nested. Sparse is better than dense. Readability counts. Special cases aren't special enough to break the rules. Although practicality beats purity. Errors should never pass silently. Unless explicitly silenced. In the face of ambiguity, refuse the temptation to guess. There should be one-- and preferably only one --obvious way to do it. Although that way may not be obvious at first unless you're Dutch. Now is better than never. Although never is often better than *right* now. If the implementation is hard to explain, it's a bad idea. If the implementation is easy to explain, it may be a good idea. Namespaces are one honking great idea -- let's do more of those! >>>

more specifically the two points,

Explicit is better than implicit

Readability counts.

It is more correct and more elegant (imho), because it also checks the type, like the following

>>> True is True True >>> bool([1]) True >>> bool("hello") True >>> bool(1) True >>> 1 is True <stdin>:1: SyntaxWarning: "is" with a literal. Did you mean "=="? False >>> [1] is True False >>> "hello" is True <stdin>:1: SyntaxWarning: "is" with a literal. Did you mean "=="? False >>>

This is why I am used to not leave any doubt about why there should be

if len(li): pass

instead of

if li: pass

I'm not sure I agree about it being more readable. Both are quite readable. It's definitely more pedantic, and I know it enforces the type so it is more accurate. The question would be does the accuracy matter.

It sounds like the simple answer would have just been "yes", and we can just leave it as is (I know it might also be frustrating for @rootart to be asked to switch it back so they may just want to leave it as is too 😅 )

no problem. let's get this through, if you have reviewed it it's okay for me, in case I will change the tests to be more pedantic when I will have time for it

I think I might have made some mistakes, by saying that assert bool(res) is True would be the right way.
I disagree, I al in favor of assert res is True

terencehonles · 2021-10-21T23:54:07Z

tests/test_backend.py


-    @patch("django_redis.cache.DJANGO_VERSION", (3, 1, 0, "final", 0))


It should be OK to leave @patch as a decorator (here and the rest of the PR).

Alright, I will still keep mocker for patching objects and properties if possible.

terencehonles · 2021-10-21T23:58:41Z

tests/test_cache_options.py

+
+
+@pytest.fixture
+def key_prefix_cache(settings: SettingsWrapper) -> Generator[RedisCache, None, None]:


Suggested change

def key_prefix_cache(settings: SettingsWrapper) -> Generator[RedisCache, None, None]:

def key_prefix_cache(settings: SettingsWrapper) -> Iterable[RedisCache]:

terencehonles · 2021-10-22T10:05:27Z

tests/test_cache_options.py

+    caches_setting = copy.deepcopy(settings.CACHES)
+    caches_setting["default"]["KEY_FUNCTION"] = "test_cache_options.make_key"
+    caches_setting["default"]["REVERSE_KEY_FUNCTION"] = "test_cache_options.reverse_key"
+    settings.CACHES = caches_setting


I don't believe you need the copy 🤔 or is this because the settings wrapper doesn't understand mutations to the underlying dict?

Suggested change

caches_setting = copy.deepcopy(settings.CACHES)

caches_setting["default"]["KEY_FUNCTION"] = "test_cache_options.make_key"

caches_setting["default"]["REVERSE_KEY_FUNCTION"] = "test_cache_options.reverse_key"

settings.CACHES = caches_setting

settings.CACHES["default"]["KEY_FUNCTION"] = "test_cache_options.make_key"

settings.CACHES["default"]["REVERSE_KEY_FUNCTION"] = "test_cache_options.reverse_key"

Looks like that direct mutation doesn't trigger the required signals to reload the configuration.

terencehonles · 2021-10-22T10:08:54Z

tests/test_client.py

+    def test_close_disconnect_settings_cache(
+        self, cache_client: DefaultClient, mocker: MockerFixture
+    ):
+        settings.CACHES[DEFAULT_CACHE_ALIAS]["OPTIONS"]["CLOSE_CONNECTION"] = True


Thinking about the last comment, does settings need to be a fixture so that this doesn't affect the global state for this config?

You are right, will replace it with a fixture to avoid changing the global state.

…nction test classes to pytest

…tShardClient classes to pytest

terencehonles · 2021-10-25T11:09:26Z

I reviewed everything and made some minor changes, but it looks good @rootart! Thanks for all your efforts 🙂

WisdomPill requested changes Oct 9, 2021

View reviewed changes

rootart force-pushed the pytest-migration branch from 812cb2e to c9e06e1 Compare October 10, 2021 10:09

rootart requested a review from WisdomPill October 10, 2021 18:57

rootart force-pushed the pytest-migration branch 2 times, most recently from 4ca48a4 to 151b8e1 Compare October 11, 2021 20:46

WisdomPill changed the title ~~#521, Convert unittest class base tests to pytest function tests~~ Convert unittest class base tests to pytest function tests Oct 19, 2021

terencehonles reviewed Oct 22, 2021

View reviewed changes

dizhakbot added 10 commits October 23, 2021 20:09

Migrate DjangoRedisConnectionStrings, DjangoRedisCacheTestCustomKeyFu…

d45becb

…nction test classes to pytest

Migrate TestDjangoRedisCache, TestClientClose, TestDefaultClient, Tes…

7d49bb2

…tShardClient classes to pytest

Fix migration from assertAlmostEqual to pytest.approx

d0fe36d

Convert to using mocker instead of unittest.mock

aa7ed31

Fix linting

672d30a

Split session tests into separate file

c1e2dfb

Split tests into separate modules

24b2518

Fix review comments

1ee5b55

Add type annotations to the tests

39be0fb

Fixes after review and rollback usages of mocker.patch

2b820be

rootart force-pushed the pytest-migration branch from 151b8e1 to 2b820be Compare October 23, 2021 19:10

rootart requested a review from terencehonles October 23, 2021 19:10

terencehonles added 4 commits October 25, 2021 11:51

add news fragment

8bf239d

commit typing suggestion

a12711f

various fixes

f10c094

add missed conftest.py

ae30ded

run tox env mypy on tests

24f6c8a

terencehonles merged commit 5bf67ab into jazzband:master Oct 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert unittest class base tests to pytest function tests #553

Convert unittest class base tests to pytest function tests #553

rootart commented Oct 9, 2021 •

edited by WisdomPill

Loading

codecov bot commented Oct 9, 2021 •

edited

Loading

WisdomPill left a comment

WisdomPill Oct 9, 2021

WisdomPill Oct 9, 2021

WisdomPill Oct 9, 2021

WisdomPill Oct 9, 2021

WisdomPill Oct 9, 2021

WisdomPill Oct 9, 2021

WisdomPill Oct 9, 2021

rootart Oct 9, 2021

WisdomPill Oct 9, 2021

WisdomPill Oct 9, 2021

rootart Oct 10, 2021

WisdomPill Oct 9, 2021

rootart commented Oct 9, 2021

WisdomPill commented Oct 10, 2021

rootart commented Oct 11, 2021

rootart commented Oct 21, 2021 •

edited

Loading

terencehonles commented Oct 21, 2021

terencehonles left a comment

terencehonles Oct 21, 2021

terencehonles Oct 21, 2021

WisdomPill Oct 22, 2021

terencehonles Oct 22, 2021 •

edited

Loading

WisdomPill Oct 22, 2021

WisdomPill Oct 22, 2021

terencehonles Oct 21, 2021

rootart Oct 23, 2021

terencehonles Oct 21, 2021

terencehonles Oct 22, 2021

rootart Oct 23, 2021

terencehonles Oct 22, 2021

rootart Oct 23, 2021

terencehonles commented Oct 25, 2021

	def cache() -> Generator[RedisCache, None, None]:
	def cache() -> Iterable[RedisCache]:


		@patch("django_redis.cache.DJANGO_VERSION", (3, 1, 0, "final", 0))



		@pytest.fixture
		def key_prefix_cache(settings: SettingsWrapper) -> Generator[RedisCache, None, None]:

Convert unittest class base tests to pytest function tests #553

Convert unittest class base tests to pytest function tests #553

Conversation

rootart commented Oct 9, 2021 • edited by WisdomPill Loading

codecov bot commented Oct 9, 2021 • edited Loading

Codecov Report

WisdomPill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rootart commented Oct 9, 2021

WisdomPill commented Oct 10, 2021

rootart commented Oct 11, 2021

rootart commented Oct 21, 2021 • edited Loading

terencehonles commented Oct 21, 2021

terencehonles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terencehonles commented Oct 25, 2021

rootart commented Oct 9, 2021 •

edited by WisdomPill

Loading

codecov bot commented Oct 9, 2021 •

edited

Loading

rootart commented Oct 21, 2021 •

edited

Loading

terencehonles Oct 22, 2021 •

edited

Loading