Skip to content
This repository was archived by the owner on Mar 19, 2021. It is now read-only.

Conversation

@nadaa
Copy link
Contributor

@nadaa nadaa commented Oct 2, 2018

This to fix issue #23
Add a unittest for formatter.py file.

This is a basic unittest to test the four formatters (table,json,tab,markdown). It could be better if the test data is stored in an external file. Also adding more test cases. Tabformatter only expects the input to be a list, so I didn't include its test. for now

@@ -0,0 +1,114 @@
import pytest
Copy link
Member

@ahal ahal Oct 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are two issues with the encoding:

  1. On python 2, it's failing because you're using unicode characters in literal strings. I think if you add from __future__ import unicode_literals to the very top of your file you'll stop seeing the SyntaxError.

  2. The assertion is failing in python 3 (and will likely also fail in python 2 once you fix the first issue). I think this is because the literal is a string type, but the value that your computing from the formatter is bytes.

Try doing something like this:

all_formatters['table'](test_data).decode('utf8', 'replace')

If that doesn't work, try printing the types of both values:

print(type(formatter...))
print(type(expected_table))

They'll need to match up and that will help us figure out why it's failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ahal , I tried to remove py2 from tox.ini file, and everything works. I don't know why py2 cause assertion errors!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, that's really strange! I'll pull this down and poke around a bit when I get a chance. Maybe we should just drop python 2 support anyway :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you make any changes to this PR? I pulled this down and removed the python 2 from tox.ini, but the tests still fail for me afterwards.

Maybe we should move this into data files rather than storing them in the test file. That should help fix the python 2 errors. Then the python 3 errors should just be a matter of decoding/encoding both sets of strings properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't change the file recently, I rechecked tox, see the screen below:

I will work on separating data from tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am working on this, first, separated the test data into json files for each formater. tempdir fixture was used to read the files, then, saved the data into a list. Now, I want to pass as a parameter to another fixture, which will pass each fomatter test cases to a test function, still couldn't make it, but I will keep trying.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @ahal , the problem still exists even after putting the test data outside. I think it is a bug in tox, python 3 works fine but not py2, please, have a look at tox log with only py 3 and py2+py3
https://gist.github.com/nadaa/432eb32c9efd0074f9608929b6233b0d

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks for the attempt, I'll try again in a bit (I've been out most of the week so haven't much time to investigate too far yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're welcome. Shall I push the update?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants