Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixtures tester improvements #19

Merged
merged 6 commits into from
May 24, 2013

Conversation

masklinn
Copy link
Contributor

Builds on top of #18:

  • Removes unimplemented visit_ and depart_ methods, on grounds that it's easier to just blow up than have garbage output
  • Alters WriterTestCase to reify each fixture file as a test case:
    • Better overview/progress notifications
    • Failure of a fixture does not preclude trying out other fixtures

WriterTestCase is 2.6+ (2.5 won't fail but won't find any test case), though most of the complexity comes from remaining compatible with 2.6: Python 2.7 merged unittest2 and introduced the load_tests protocol but in Python 2.6 autodiscovery of dynamic test cases requires:

  • overriding __dir__ so a sequence of test_$thing names can be returned to the test loader (on the TestCase subclass itself because the test loader calls dir(testCaseClass), thus on a metaclass of the test case)
  • overriding __getattr__ on both the type (because unittest will immediately try to access the name it got on the class itself to check it's callable) and the instance (to provide the actual runner for the corresponding fixture)
  • run_fixture has the job of actually running the fixture

--HG--
rename : rst2rst/tests/__init__.py => rst2rst/tests/test_fixtures.py
this way, removals and additions show the *errors* in the current output
Allows checking multiple failing fixtures, and better reporting on
tests count.
better to fail on them, if no-behavior is desirable should extend SparseNodeVisitor instead
@benoitbryon
Copy link
Owner

Question: how do you run the tests?
I use make test (make configure develop test after a new clone).

When I merge this pull request, make test runs 4 tests (all pass), whereas, before the merge, make test runs 6 tests (1 fails). If this is an expected result of this pull-request, I guess I misunderstood something, so tell me ;)

Move tests from tests/init so unittest2's

Are you using unittest2's test loading? Is that a nose feature?

Have each fixture be reported as its own test case

Nice!

most of the complexity comes from remaining compatible with 2.6

Could https://pypi.python.org/pypi/unittest2 help?
I am ok to add a dependency to unittest2 for Python < 2.7 if it helps us have clean and easy to maintain code.
That said, it could be implemented in another ticket: I may accept this pull request to get things done, then, have another ticket to improve.

benoitbryon added a commit that referenced this pull request May 21, 2013
… rst2rst.tests.packaging ; some PEP8-related changes.
@masklinn
Copy link
Contributor Author

I use make test (make configure develop test after a new clone).

Ah I'll have to look into the target, I didn't consider you'd be using that.

If this is an expected result of this pull-request

I feel pretty sure that it's not, there should be more tests reported after than before.

Are you using unittest2's test loading? Is that a nose feature?

Nose has that, but it's also been added to Python 2.7+'s unittest: http://docs.python.org/2/library/unittest.html#test-discovery. And it's been backported to Python 2.4+ as unittest2: https://pypi.python.org/pypi/unittest2

Could https://pypi.python.org/pypi/unittest2 help?

Yeah, that would make the load_tests protocol available and it would be possible to create a custom test suite instead of hacking around with the introspection stuff. I think it'd end up shorter and more readable. It should also restore Python 2.5 compat' (if the package was compatible before, not sure about that)

@benoitbryon
Copy link
Owner

And what about nose's test generators? http://nose.readthedocs.org/en/latest/writing_tests.html#test-generators
Could be even more simple to develop and maintain?
Again, this may be implemented as an improvement in another ticket.
Note: the development environment already depends on nose. See

and
$(NOSE) --config=etc/nose.cfg --with-doctest --with-coverage --cover-erase --cover-package=rst2rst rst2rst
... It means that we can already use nose's internals.

I use make test (make configure develop test after a new clone).

Ah I'll have to look into the target, I didn't consider you'd be using that.

Tell me what you are running, so that we can adapt the make test accordingly.
What seems important to me is that every developer can run the tests using the same "interface", i.e. running make test. Then make test implementation can change, while it provides good user experience.

If this is an expected result of this pull-request

I feel pretty sure that it's not, there should be more tests reported after than before.

Ok, so I currently have an issue with the test loading.
I'm trying some things and inspection to figure out what is going wrong. Any clue is welcome ;)
pdb tells the WriterTestCase class has "test_bulletlist" method.

@benoitbryon
Copy link
Owner

It should also restore Python 2.5 compat' (if the package was compatible before, not sure about that)

I did nothing about compatibility for multiple Python version. My priority was to implement the functionality for some python version (the one that is installed by default on my computer).
That said, I agree compatibility matters. Even more since several developers work on the project :)
I created #24 and #26 about this topic.
I propose we do minimal actions to run the tests on our computers (@masklinn and @benoitbryon), so that we can collaborate and get this pull-request merged. Then, later, #24 and #26 will help us check, fix and maintain compatibility.

@benoitbryon
Copy link
Owner

Wouldn't something like this be more readable?

class TestMeta(type):
    def __new__(mcs, name, bases, dict):
        for name in fixture_names():
            dict['test_{name}'.format(name=name)] = lambda self: self.run_fixture(*self.fixture_paths(name))
        return type.__new__(mcs, name, bases, dict)

I feel this would be a fair replacement for both __dir__ and __getattr__: test methods would be added statically on class creation, instead of being computed dynamically via __dir__ or __getattr__. Would it break something?

@benoitbryon
Copy link
Owner

I can run the tests with bin/python -m unittest discover -v -p '*', which uses unittest2 (with Python 2.7).
But I couldn't get it work with nose :(
Seems that my issue is related to nose's test discovery...

benoitbryon added a commit that referenced this pull request May 22, 2013
… test_* methods, fixed compatibility with nose and unittest2 (Python 2.7).
@benoitbryon
Copy link
Owner

I hope I got it!
@masklinn: can you review 5548e89 and try the https://github.com/benoitbryon/rst2rst/tree/19-fixtures-tester-improvements branch?

git remote add upstream git@github.com:benoitbryon/rst2rst.git
git fetch upstream
git checkout -b 19-fixtures-tester-improvements upstream/19-fixtures-tester-improvements
make develop
make test
bin/python -m unittest discover -p '*' -v

The last 2 lines should give the following results:

unittest: 7 tests run, 1 failed
make test: 8 tests run, 1 failed (it runs 1 doctest that unittest doesn't captures)

If it works for you, let's close this ticket and move forward to the other pull-requests :)
Later, we will ensure compatibility with Python < 2.7 with #26.

@benoitbryon
Copy link
Owner

Note: I used a function-based metaclass, because, with nose, TestMeta.new() was not triggered (I do not know why)... with a function-based metaclass, it seems to work with both nose and unittest (Python >= 2.7)

@masklinn
Copy link
Contributor Author

Seems to work here as well, though if rst2rst/tests/packaging.py was renamed to e.g. test_packaging.py the unittest discover call wouldn't even need the -p argument.

And switching to metaclasses means it works again in Python 2.6 (using either nose or unittest2), cools stuff.

One thing though: in fixture_names, I don't think there's any point in sorting the method names, the class dictionary isn't an ordered dict so the order's lost (whereas __dir__ yields a list so it sort-of made sense). re.sub(r'^(.+?)-input\.txt$', r'\1', f) can probably be replaced by re.match(r'^(.+?)-input\.txt$', f).group(1), I must have been drunk when I wrote that.

It almost works in Python 2.5 too: there are a pair of with which require from __future__ import with_statement imports, and 3 str.format() calls which would have to be replaced by %

benoitbryon added a commit that referenced this pull request May 24, 2013
benoitbryon added a commit that referenced this pull request May 24, 2013
@benoitbryon
Copy link
Owner

Seems to work here as well, though if rst2rst/tests/packaging.py was renamed to e.g. test_packaging.py the unittest discover call wouldn't even need the -p argument.

Done.
Notice that tests are to be discovered with nose. Support of unittest2 is not mandatory for now.
I mean, what matters here and now is that make test runs all the tests. It does.
That said, I did the change because it is unittest2 default pattern, perhaps it is kind of convention... whenever I'd prefer removing this meaningless "test_" prefix (those modules are in "tests" package!). This point may be refactored one day. Anyway, it is no big deal ;)

It almost works in Python 2.5 too: there are a pair of with which require from future import with_statement imports, and 3 str.format() calls which would have to be replaced by %

Let's do it in another ticket, where we focus on compatibility with Python 2.5.
Right now, there is no easy way to test against several Python versions (i.e. make test only runs with current Python version).

benoitbryon added a commit that referenced this pull request May 24, 2013
@benoitbryon benoitbryon merged commit dc83b94 into benoitbryon:master May 24, 2013
@masklinn masklinn deleted the fixtures-tester-improvements branch November 14, 2017 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants