test: Use pytest for testing #121

abhiabhi94 · 2020-12-13T14:52:23Z

also fix some tests that required patching settings.
reducing time for tox tests by running it for lesser environments(still cover all versions)
add support for python3.9

- also fix some tests that required patching settings. - reducing time for tests running on travis by running them for lesser environments(still cover all versions) - add support for python3.9 fix Radi85#119 fix Radi85#112

Radi85

In general I have compared current test with pytest and the current test is still faster. So do you think we need pytest anymore?!
Also can you please remove the text related to the icon from your commit? It's nice on github but let's try to not spam the commits/git history with icon text.

Radi85 · 2020-12-14T07:44:09Z

comment/tests/test_models.py

@@ -40,16 +40,16 @@ def test_is_edited_for_anonymous_comment(self):
        comment = self.create_anonymous_comment(posted=timezone.now() - timezone.timedelta(days=1))
        self.assertFalse(comment.is_edited)

+    @patch.object(settings, 'COMMENT_FLAGS_ALLOWED', 1)


COMMENT_FLAGS_ALLOWED is a boolean so please use True instead of 1

no, it is not.

you are right!
The variable name points to a boolean value. I think this name would be more descriptive COMMENT_MAX_FLAGS_NUMBER_ALLOWED

The variable name points to a boolean value.

something like COMMONT_IS_FLAG_ALLOWED appears more outrightly boolean to me.

I think this name would be more descriptive COMMENT_MAX_FLAGS_NUMBER_ALLOWED

this seems to be more of a whimsical comment to me 😆 , COMMENT_MAX_FLAG_ALLOWED seems better to me but any such change will require a major version bump. Also, if we go down this route, maybe we might need to change almost all of the defaults.

Radi85 · 2020-12-19T10:38:52Z

comment/tests/test_utils.py


    def test_id_generator_generates_different_ids(self):
-        self.assertNotEqual(id_generator(), id_generator())
+        assert id_generator() != id_generator()


can you please change this test as suggested to get rid of SonarLint error

Suggested change

assert id_generator() != id_generator()

first_id = id_generator()

second_id = id_generator()

assert first_id != second_id

Radi85 · 2020-12-19T10:46:41Z

comment/tests/test_utils.py

@@ -337,36 +336,34 @@ def test_for_drf(self):
        self.assertEqual(EmailInfo.CONFIRMATION_SENT, response)


-class UtilsTest(TestCase):
+class TestUtils:


Why TestCase is removed from this test only? I would prefer using the self.assert* rather than python assert keyword.

Why TestCase is removed from this test only?

this is a luxury that you can use with pytest. You don't need to pass unittest's TestCase or even django's TestCase when using pytest-django, it does it for you. You can read more about it in their documentation https://docs.pytest.org/en/latest/.

There is a bit of magic, it doesn't necessarily use python's assert statement. It automatically does some magic behind the scenes and does all the comparison for you so that you don't have to remember and write different statements(self.assertEqual, self.assertTrue) for different use-cases.

I would prefer using the self.assert* rather than python assert keyword.

If that is the case, there is very little point in switching to the pytest. Feel free to close this one then.

I see that the form of assertion self.assertEqual and self.assertTrue is more explicit on the type of the value that's why I would prefer it.
Moreover, I wouldn't prefer to stick with one testing framework.

You have already done the work for #112, if you like you can just revert pytest changes and we can still merge this PR.

The current change to pytest was made after the issue was discussed. Reverting things because one of us doesn't like the change, seems a lot of waste of time for both of us. This also makes the process more irritating than enjoyable for me.

I think we need to build consensus and be more clear before any code is written so that both of us doesn't waste time.

First of all I would appreciate you contribution to the project.
There should not be an issue if we try something and revert it after finding out that it doesn't fit our need.
In regard to the pytest, I have told you that I never used this framework and didn't have time to fiddle with it before you implement the changes so I am sorry to waste your time.

Lastly, I think it is a good time to create some labels to tag issues that are ready for implementation to avoid wasting people's time :)

abhiabhi94 · 2020-12-20T02:01:55Z

In general I have compared current test with pytest and the current test is still faster. So do you think we need pytest anymore?!

In order to reduce the time, we will need to re-write most of the current tests. I don't think I meant that at any stage. What I meant was if we need pytest's functionality to write newer tests, they can be faster and cleaner.

Also can you please remove the text related to the icon from your commit? It's nice on github but let's try to not spam the commits/git history with icon text.

okay 😢, although spam maybe is too strong a word. if it were "spam", it probably wouldn't have been used in framework as popular as fastapi(https://github.com/tiangolo/fastapi)

Radi85 · 2020-12-20T08:30:15Z

okay 😢, although spam maybe is too strong a word. if it were "spam", it probably wouldn't have been used in framework as popular as fastapi(https://github.com/tiangolo/fastapi)

I have two points on this topic:

If a style or tech is used in another project, that doesn't mean it is a GOOD choice.
Using this fancy icon requires special fonts for the terminal, below is the difference between our git tree and fastapi git tree.

Fastapi:

* 4fdcdf3 (HEAD -> master, origin/master, origin/HEAD) 📝 Update release notes
* e2a6341 📝 Update Uvicorn installation instructions to use uvicorn[standard] (includes uvloop) (#2543)
* 7046d80 📝 Update release notes
* 9f89399 🌐 Add docs lang selector widget (#2542)
* ed0fe9f 📝 Update release notes
* 6e9b771 🐛 Fix docs overrides directory for translations (#2541)

Our tree:

* 558ec50 (origin/master, origin/HEAD, master) :recycle: refactor: Clean up views
| * e9b43f1 (pytest) :white_check_mark: test: Use pytest for testing
|/  
* 1c2359e (tag: v2.5.1, staging) fix(#114): fix version issue
* 6de6055 fix: rename user and profile serializers
* 7ca4f11 update issue templates

I don't like to see such commit :white_check_mark: test: Use pytest for testing in the git history
while I don't mind with this 📝 test: Use pytest for testing.

abhiabhi94 · 2020-12-20T09:26:10Z

If a style or tech is used in another project, that doesn't mean it is a GOOD choice.

I never said anything about this being good or bad. You might be going down the route of Strawman argument.

I don't like to see such commit ✅ test: Use pytest for testing in the git history
while I don't mind with this 📝 test: Use pytest for testing.

I will do it as you say, but maybe I want more creative freedom around the project. I do agree that the icon text just has somewhat of overhead, but I like it 😭

abhiabhi94 · 2020-12-25T13:33:18Z

@Radi85 it seems wise to me to close this pull-request and make a new one for #112

I'm doing as of what you say, but pytest seems a cleaner framework to me to write tests. Maybe you too will realize this someday and remember the day I said this 😆

Also, jokes apart some current tests were actually passing because of the sequence in which django's unittest runs them. Independently they should have been patched with mock settings. I will see that I fix that too in the newer one.

Lastly, one more thing, and I kind of feel a bit weird doing this, but wanted to tell you that I was looking for a job. It would be great of you, if you have any opening where you can refer me.

abhiabhi94 changed the title ~~✅ test: Use pytest for testing~~ test: Use pytest for testing Dec 13, 2020

abhiabhi94 force-pushed the test/move-to-pytest branch 2 times, most recently from 123d9f9 to 89ba141 Compare December 13, 2020 15:00

abhiabhi94 requested a review from Radi85 December 13, 2020 15:00

abhiabhi94 force-pushed the test/move-to-pytest branch from 89ba141 to 134ece6 Compare December 13, 2020 15:06

✅ test: Use pytest for testing

e9b43f1

- also fix some tests that required patching settings. - reducing time for tests running on travis by running them for lesser environments(still cover all versions) - add support for python3.9 fix Radi85#119 fix Radi85#112

abhiabhi94 force-pushed the test/move-to-pytest branch from 134ece6 to e9b43f1 Compare December 14, 2020 03:36

Radi85 requested changes Dec 19, 2020

View reviewed changes

abhiabhi94 closed this Dec 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Use pytest for testing #121

test: Use pytest for testing #121

abhiabhi94 commented Dec 13, 2020

Radi85 left a comment •

edited

Loading

Radi85 Dec 14, 2020

abhiabhi94 Dec 20, 2020

Radi85 Dec 20, 2020

abhiabhi94 Dec 20, 2020

Radi85 Dec 19, 2020

Radi85 Dec 19, 2020

abhiabhi94 Dec 20, 2020

Radi85 Dec 20, 2020

abhiabhi94 Dec 20, 2020

Radi85 Dec 20, 2020

abhiabhi94 commented Dec 20, 2020 •

edited

Loading

Radi85 commented Dec 20, 2020 •

edited

Loading

abhiabhi94 commented Dec 20, 2020 •

edited

Loading

abhiabhi94 commented Dec 25, 2020

-        assert id_generator() != id_generator()
+        first_id = id_generator()
+        second_id = id_generator()
+        assert first_id != second_id

test: Use pytest for testing #121

test: Use pytest for testing #121

Conversation

abhiabhi94 commented Dec 13, 2020

Radi85 left a comment • 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

Choose a reason for hiding this comment

abhiabhi94 commented Dec 20, 2020 • edited Loading

Radi85 commented Dec 20, 2020 • edited Loading

abhiabhi94 commented Dec 20, 2020 • edited Loading

abhiabhi94 commented Dec 25, 2020

Radi85 left a comment •

edited

Loading

abhiabhi94 commented Dec 20, 2020 •

edited

Loading

Radi85 commented Dec 20, 2020 •

edited

Loading

abhiabhi94 commented Dec 20, 2020 •

edited

Loading