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

Move doctests to unittests #17

Merged
merged 13 commits into from
Dec 28, 2015
Merged

Conversation

moyogo
Copy link
Collaborator

@moyogo moyogo commented Nov 12, 2015

  • doctests from ufornormalizer.py have been moved to test_ufornormalizer.py as unittests
  • ufonormalizer -t calls unittest instead of doctest
  • use plistlib.loads() and .dumps() if available (Python 3.4 deprecated plistlib.readPlistFromBytes(), .writePlistToBytes()` and prints warnings when used)
  • add Python 3.5 to Travis-CI file
  • add AppVeyor file for Windows testing

@typesupply
Copy link
Contributor

Why shouldn't these be in doctests?

@moyogo
Copy link
Collaborator Author

moyogo commented Nov 12, 2015

It’s fine to have example tests in the docstring but unittest is better for thorough testing, doctest is more appropriate for documenting or basic testing. The docstrings should be short and to the point, they end up in .doc. Having the unit tests in a separate file makes the main script lighter.

There are also several issues with using only doctests, unittests are just more flexible and can be more atomic.

@anthrotype
Copy link
Member

@typesupply
Copy link
Contributor

Yes, but...

This isn't a typical module that needs atomic documentation. There is commandline behavior and a single public function. The rest is all internal.

I don't have anything against unittests. In this case, I put everything in doctests because it made sense to put the tests next to the code that they are evaluating. In almost all of the functions in this file the tests were the documentation. Now things like xmlConvertInt have no documentation or information about how they are expected to handle certain situations. The file may be cleaner, but it is going to be harder to make changes without cross checking.

I'll think about this switch. I'm not rejecting it. I just need to think about it.

@moyogo moyogo force-pushed the unittest branch 2 times, most recently from f958b37 to eed2e21 Compare November 12, 2015 18:11
@typesupply
Copy link
Contributor

Alright, I decided to merge this just to keep the main file shorter. Could you resolve the conflicts and update the PR?

@moyogo
Copy link
Collaborator Author

moyogo commented Dec 14, 2015

OK. I'll update the PR next week.

@moyogo
Copy link
Collaborator Author

moyogo commented Dec 28, 2015

@typesupply OK, the branch has been rebased and adff343 reflect the changes made in the doctests.

typesupply added a commit that referenced this pull request Dec 28, 2015
Move doctests to unittests
@typesupply typesupply merged commit d81b24e into unified-font-object:master Dec 28, 2015
@moyogo moyogo deleted the unittest branch June 21, 2016 08:45
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