Use pytest, put tests into separate dir #70

alisianoi · 2017-06-17T10:47:20Z

I am having some problems with batavia so I decided to sidestep a little and just shuffle things around in boolean.py, will later be getting back to making batavia work.

A thing to watch out for in this PR: readme display on pypi website might break because they use a rather strict and custom (afaik) parser for .rst and .md there. Once this lands, I will keep an eye on that and fix it if need be.

Refs: pyenv/pyenv-virtualenv#206 Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

pombredanne · 2017-06-19T15:01:58Z

Is the init.py in https://github.com/bastikr/boolean.py/pull/70/files#diff-498cf53d35427897613cdfc4b76fc6ea really needed?

pombredanne

I added some comments. The thing that makes it quite hard to review is that most everything has changed in the tests.
I am ok with using py.test as it makes things cleaner, but being able to review the diff of a test before and after would make much more sense to review anything correctly.
Do you think you could merge it all back as a single test file (and keep the original ordering of the tests) ...?

Once that is done, splitting in files would be a non issue, so as they say flat is better than nested

pombredanne · 2017-06-19T14:26:41Z

tests/test_not.py

+
+        assert a == a.simplify()
+        assert a == (~~a).simplify()
+        assert a == (~~ ~~a).simplify()


It looks like you injected a space here and may be in a few other ~~ places... likely a side effect of your code editor or formatter?

Actually, no, that space was intentional. It was there before:

bfb0b31#diff-b03f7319ce9e9d763799954c9b76efa9L241

pombredanne · 2017-06-19T14:29:11Z

test-requirements.txt

@@ -1 +1,4 @@
 tox==2.7.0
+pytest==3.1.2


Any reason why you need such strict pinned requirements?

No, not really. Just in the spirit of exactly pinning dependencies. Later on, one could outsource dependency updating to a bot like https://pyup.io/ which will create PRs when a dependency could be updated. However, can change to 3.1.x or even 3.x.x (not sure about the exact syntax though)

pombredanne · 2017-06-19T14:30:05Z

setup.py

-'''
-
+with open('README.md') as readme:
+    long_description = readme.read()


You could use an .rst file and use restview to check that it works fine with Pypi?

That is true, will rewrite the readme in .rst and check it then.

pombredanne · 2017-06-19T15:03:29Z

tests/test_not.py

+        assert (~(a & a & a)).simplify() == (~(a & a & a)).simplify()
+        assert (~(a | a | a)).simplify() == (~(a | a | a)).simplify()
+
+    @pytest.mark.xfail(reason="a.cancel() should work but does not")


You have changed the logic of this test.
It was:

def test_cancel(self): algebra = BooleanAlgebra() a = algebra.Symbol('a') self.assertEqual(~a, (~a).cancel()) self.assertEqual(a, algebra.parse('~~a').cancel()) self.assertEqual(~a, algebra.parse('~~~a').cancel()) self.assertEqual(a, algebra.parse('~~~~a').cancel())

So I am not sure cancelling a symbol makes any sense. Also the logic to parse vs. constrict Python objects is rather different... so in changing the test you are not testing the same thing (eventually both would be good, though a primary use case is with a parsed input)

My thinking process there was:

there is a separate test that deals exclusively with parse and also parsing is never easy, so let's throw it out of this test and just use python objects

I also started a separate test_parse.py later, moving a small portion of test_parse from OtherTestCase . The move is not finished yet though.

Resolution: I will write a second version of test_cancel that uses parse then.

alisianoi · 2017-06-19T16:01:25Z

Sure, I can first do the changes in the same file and in the same order so that it would be easier to review. However, please note that you could:

instead of clicking on "Files changed" to see all the changes at once, click on the individual commits. That pulls up the diff for the exact test I was moving around/modifying.
do a similar thing locally in console by git diff hash0..hash1. If you pick hashes of consecutive commits, you see exactly the difference between them.

Anyway, I totally understand that this PR is big/hard to follow. As you suggest, I can use it to quick-start another PR that would first do the changes and then (once you've seen the exact changes), splits the files.

alisianoi · 2017-06-19T16:02:37Z

Is the init.py in https://github.com/bastikr/boolean.py/pull/70/files#diff-498cf53d35427897613cdfc4b76fc6ea really needed?

Removing the file and runnign pytest gives an import error. Tell me if you do not like the empty __init__.py, maybe it is possible to remove it and modify the imports somehow.

* Use "README.md as long_description" common practice * Fix license name: LICENSE is *simplified*, not *revised* BSD, see: https://en.wikipedia.org/wiki/BSD_licenses Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Use link below to verify setup: https://docs.pytest.org/en/latest/goodpractices.html Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Aim of test_parse.py is to (later) supersede OtherTestCase.test_parse Better parsing is required due to these issues: Refs: #60 Refs: #61 Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

alisianoi · 2017-06-21T10:56:56Z

And I merged back the test_symbol.py, test_not.py, and test_base_element.py. Hope it makes things easier to review.

Did not merge back mock_advanced_algebra.py and mock_custom_algebra.py because the whole point was to put those classes into separate mock files.

Also did not merge back test_parse.py because those are new tests for the parse functionality, so I am writing them in a separate file.

Also rewrote README.md as README.rst, added some badges and checked it with restview and python setup.py check --restructuredtext -- seems ok.

Update: the things I did not do yet:

write a second test that uses parse as before, Use pytest, put tests into separate dir #70 (comment)

Even though some places recommend fixing dependencies [1], if you perform a github search [2] you will see that those repositories do *not* pin their versions. So, unpinning versions here. [1]: https://devcenter.heroku.com/articles/python-pip#best-practices [2]: https://github.com/search?utf8=%E2%9C%93&q=language%3Apython+stars%3A%3E1000 Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Test .cancel() on both Python variables and .parse() results [1]: #70 (comment) Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

alisianoi · 2017-06-29T08:54:07Z

Everything has been addressed, I am removing the [WIP] prefix and waiting for a review.

pombredanne · 2017-06-30T13:58:08Z

setup.py

    author='Sebastian Kraemer',
    author_email='basti.kr@gmail.com',
    url='https://github.com/bastikr/boolean.py',
    packages=find_packages(),
    include_package_data=True,
    zip_safe=False,
-    test_loader='unittest:TestLoader',


The previous test suite could run equally well with or without pytest. Any special reason to make py.test a hard requirement for tests?

Just to force everybody to use the same thing. The more options you have, the more (compatibility?) things you have to keep in mind. With just one thing there is only one way: pip install -r requirements.txt; pytest

(I know this is a very old PR, but...)

The use of @pytest decorators makes pytest a hard requirement, full stop. (And I agree with @alisianoi: it's better not to support multiple test frameworks, which lead to everyone executing the tests in subtly different ways. Do that, you've stopped testing your code; now you're compatibility-testing the frameworks instead.)

pytest-runner is deprecated. (As are any use of test_runner, test_suite, tests_require, etc. in setup.py. setup.py as a whole is deprecated, of course, but its test args are "super double deprecated".) These days, tox would be a better choice as test runner.

pombredanne

Thanks for remerging the tests: this was much easier to review!

pombredanne · 2017-06-30T14:01:02Z

tests/test_boolean.py

        algebra = BooleanAlgebra()
-        self.assertEqual(algebra.TRUE, algebra.TRUE)
-        BaseElement()


You have missed carrying over this test

Oh, you mean BaseElement(), the empty call. So, it should test that an empty call does not trigger any sort of exception?

This is the same type of test that I just removed from https://github.com/bastikr/boolean.py/pull/70/files/29e66f0e82244f90608eaad9739cf0271cb9a983#r125046762

I will look up how to write a unit test that "makes sure this is legal" (or tell me if you know how) and bring this and the linked tests from other comment back.

pombredanne · 2017-06-30T14:03:13Z

tests/test_boolean.py

+        assert algebra.FALSE == algebra.FALSE
+
+        assert     algebra.TRUE != algebra.FALSE
+        assert not algebra.TRUE == algebra.FALSE


is not == the same as != ? things are a tad complicated at times because of the overloading of dunder methods and the way Python boolean logic shortcircuiting works

Was writing like this first:

assert not (algebra.TRUE == algebra.FALSE)

and then noticed that the () can be dropped, so I dropped them. Will rewrite as:

assert algebra.TRUE != algebra.FALSE assert not (algebra.TRUE == algebra.FALSE)

pombredanne · 2017-06-30T14:04:53Z

tests/test_boolean.py


-    def test_init(self):


Where have these tests gone?

As far as I could tell, these were not tests: no assert statements of any kind. If they were to fail, that would be ugly tracebacks, not test results. Besides, that is just assignment under the hood, so don't see what is being tested.

On the other hand, the init part of the test name is confusing and might suggest that this is part of some test setup, which it is not. So, removed.

pombredanne · 2017-06-30T14:07:15Z

tests/test_boolean.py

+
+    def test_symbols_eq_0(self):
+        algebra = BooleanAlgebra()
+


Is this may be a few too many empty lines?

Actually, not really. The blank lines show three parts of the unit test: setup, run and assertion. Can remove them if you don't like them, sure.

And I could not resist:

pombredanne · 2017-06-30T16:21:47Z

tests/test_boolean.py

+        algebra = BooleanAlgebra()
+
+        assert     algebra.Symbol('a') == algebra.Symbol('a')
+        assert not algebra.Symbol('a') == algebra.Symbol('b')


same comment as above. I am not 100% sure that not == is the same as != here because of the (and, or, not) operator overloading

pombredanne · 2017-06-30T16:24:53Z

tests/test_boolean.py

+    def test_symbols_eq_ne(self):
+        algebra = BooleanAlgebra()
+
+        symbols0 = [


I tend to prefer many small tests methods that a loop... Now if we keep a loop, this still might need exploding in different purpose tests? I find it a tad hard to read what the tests are really doing

I see, this is actually something that I like to do: a few manual single unit tests above (like the test_symbol_ne* above) and then a final big test (that I also sometimes write as a test generator). If this big test starts to fail, then I find out which exact test case is failing and factor it out into yet another small, separate test_symbol_ne test. Simply factoring everything into nice small tests is too much typing and repetetive test names :)

But can do if you want, sure

pombredanne · 2017-06-30T16:29:50Z

tests/test_boolean.py

-    def test_demorgan(self):
+        assert a == a.simplify()
+        assert a == (~~a).simplify()
+        assert a == (~~ ~~a).simplify()


I think the space in the middle of the ~~ ~~ here and there is likely a typo that I introduced in the past from some editor reformatting.

pombredanne · 2017-06-30T16:31:07Z

tests/test_boolean.py

+        assert parse('~(~a & b)').demorgan() == parse('a | ~b')
+        assert parse('~(a & ~b)').demorgan() == parse('~a | b')
+
+    # TODO: enforced order is obscure, must explain


You mean the precedence of operators? I agree, this is also obscure to me

I meant the lines 585 to 595 below that use the < and > comparison operators. Is that what you meant?

pombredanne · 2017-06-30T16:32:54Z

tests/test_parse.py

+            assert algebra.parse('((' + x + '))') == algebra.TRUE
+            assert algebra.parse('(((' + x + ')))') == algebra.TRUE
+
+    @pytest.mark.xfail(reason='IndexError indicates parsing problem')


Can you elaborate on this?

If you remove the xfail decorator, the parser breaks and the whole thing crashes with an IndexError. That is not supposed to happen because the parser was designed to throw custom errors when there is a problem. So, you either throw your own errors or generic errors, not both (imho). Since it starts to throw generic errors all over sudden means there is likely some corner case that was not considered.

pombredanne · 2017-06-30T16:33:15Z

tests/test_parse.py

+            with pytest.raises(ParseError):
+                parse(expression)
+
+    @pytest.mark.xfail(reason='IndexError indicates parsing problem')


Same as above. Which index error?

alisianoi

When responding to comments, I misclicked the "Start review" button and now have to write a review summary?

Anyway, will address your comments in code, push and then enumerate the fixed/unaddressed things.

alisianoi · 2017-06-30T17:13:34Z

tests/test_boolean.py

+    def test_symbols_eq_ne(self):
+        algebra = BooleanAlgebra()
+
+        symbols0 = [


But can do if you want, sure

alisianoi · 2017-06-30T17:15:09Z

tests/test_boolean.py

+        assert parse('~(~a & b)').demorgan() == parse('a | ~b')
+        assert parse('~(a & ~b)').demorgan() == parse('~a | b')
+
+    # TODO: enforced order is obscure, must explain


I meant the lines 585 to 595 below that use the < and > comparison operators. Is that what you meant?

alisianoi · 2017-06-30T17:20:22Z

tests/test_parse.py

+            assert algebra.parse('((' + x + '))') == algebra.TRUE
+            assert algebra.parse('(((' + x + ')))') == algebra.TRUE
+
+    @pytest.mark.xfail(reason='IndexError indicates parsing problem')


If you remove the xfail decorator, the parser breaks and the whole thing crashes with an IndexError. That is not supposed to happen because the parser was designed to throw custom errors when there is a problem. So, you either throw your own errors or generic errors, not both (imho). Since it starts to throw generic errors all over sudden means there is likely some corner case that was not considered.

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

alisianoi · 2017-06-30T18:45:12Z

Ok, I have addressed:

The two places where I removed tests. They are now back with explicit exception checks.
The not and missing (), I have written explicit ()
Squashed the spaces in ~ ~~ into ~~~

For you to decide:

why pytest: Use pytest, put tests into separate dir #70 (comment)
why whitespace: Use pytest, put tests into separate dir #70 (comment)
(not) to split the big one: Use pytest, put tests into separate dir #70 (comment)
the xfail'ed IndexError: Use pytest, put tests into separate dir #70 (comment)

Also, when answering your questions, I misclicked the "Start Review" button (or something) and my last three comments ended up as a separate review. I cannot "dismiss" it, so if you could that would be great. Also, I am new to the review mechanic on github so please tell me when I am doing things wrong.

Will be removing the [WIP] and waiting for your review.

Alexander Lisianoi added 2 commits June 17, 2017 12:49

Change py36 to py361 due to a pyenv-virtualenv minor bug

23c8828

Refs: pyenv/pyenv-virtualenv#206 Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Add pytest, pytest-cov and pytest-xdist

9ef27de

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

alisianoi changed the title ~~Unit tests~~ Use pytest, put tests into separate dir Jun 17, 2017

alisianoi changed the title ~~Use pytest, put tests into separate dir~~ [WIP] Use pytest, put tests into separate dir Jun 17, 2017

pombredanne requested changes Jun 19, 2017

View reviewed changes

Alexander Lisianoi added 12 commits June 21, 2017 12:51

Change README.md to README.rst, adjust setup.py

5313d14

* Use "README.md as long_description" common practice * Fix license name: LICENSE is *simplified*, not *revised* BSD, see: https://en.wikipedia.org/wiki/BSD_licenses Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Factor test_boolean.py out into new tests directory

ce41fb3

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Make python setup.py test use pytest

46a60c5

Use link below to verify setup: https://docs.pytest.org/en/latest/goodpractices.html Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Move AdvancedAlgebra into separate mock module

00200ea

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Move CustomAlgebra into separate mock module

5f50736

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Remove misleading test_init in SymbolTestCase

7f8b037

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Improve Symbol tests for __eq__ and __ne__

39c2c7a

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Convert SymbolTestCase to pytest TestSymbol

7deb053

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Convert BaseElementTestCase to TestBaseElement, use pytest

d4717e6

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Convert NOTTestCase to TestNOT, use pytest

0cdbfee

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Add an expansion of OtherTestCase.test_parse as test_parse.py

775d8e4

Aim of test_parse.py is to (later) supersede OtherTestCase.test_parse Better parsing is required due to these issues: Refs: #60 Refs: #61 Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Remove unused maxDiff variable

c27119a

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Alexander Lisianoi added 2 commits June 29, 2017 10:28

Improve .cancel() tests as per [1]

29e66f0

Test .cancel() on both Python variables and .parse() results [1]: #70 (comment) Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

alisianoi changed the title ~~[WIP] Use pytest, put tests into separate dir~~ Use pytest, put tests into separate dir Jun 29, 2017

pombredanne reviewed Jun 30, 2017

View reviewed changes

alisianoi commented Jun 30, 2017

View reviewed changes

alisianoi changed the title ~~Use pytest, put tests into separate dir~~ [WIP] Use pytest, put tests into separate dir Jun 30, 2017

Use parentheses if an expression has 'not' in front

d5eee7e

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Alexander Lisianoi added 2 commits June 30, 2017 20:26

Make it clear which BaseElement and Symbol calls work

747764a

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

Squash spaces between tildas in .cancel(), reposition TODO comment

ac88b91

Signed-off-by: Aleksandr Lisianoi <all3fox@gmail.com>

alisianoi changed the title ~~[WIP] Use pytest, put tests into separate dir~~ Use pytest, put tests into separate dir Jun 30, 2017

Use pytest, put tests into separate dir #70

Are you sure you want to change the base?

Use pytest, put tests into separate dir #70

Conversation

alisianoi commented Jun 17, 2017 • edited Loading

pombredanne commented Jun 19, 2017

pombredanne 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

pombredanne Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alisianoi commented Jun 19, 2017

alisianoi commented Jun 19, 2017

alisianoi commented Jun 21, 2017 • edited Loading

alisianoi commented Jun 29, 2017

pombredanne Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pombredanne 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

alisianoi Jun 30, 2017 • 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

Choose a reason for hiding this comment

alisianoi 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

alisianoi commented Jun 30, 2017 • edited Loading

alisianoi commented Jun 17, 2017 •

edited

Loading

pombredanne Jun 19, 2017 •

edited

Loading

alisianoi commented Jun 21, 2017 •

edited

Loading

pombredanne Jun 30, 2017 •

edited

Loading

alisianoi Jun 30, 2017 •

edited

Loading

alisianoi commented Jun 30, 2017 •

edited

Loading