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

TST: Add input validation tests #1420

Merged
merged 4 commits into from Feb 19, 2016

Conversation

Projects
None yet
3 participants
@sandhujasmine
Contributor

sandhujasmine commented Feb 17, 2016

Fixes GH 1418

Issue status

with pytest.raises(TypeError):
b = symbol('b', 'var * {x: string}')

This comment has been minimized.

@llllllllll

llllllllll Feb 17, 2016

Member

We should move this out of the context manager. As a general rule context invariant code should not be placed in the context manager when possible because then readers need to know if the context manager affects the code in any way. In this particular case, if symbol raises a TypeError then this test will pass when it shouldn't

This comment has been minimized.

@sandhujasmine

sandhujasmine Feb 17, 2016

Contributor

Indeed - thanks @llllllllll
Fixed and tested.

@llllllllll

This comment has been minimized.

Member

llllllllll commented Feb 17, 2016

How do you feel about testing the contents of the error strings?
something like:

with pytest.raises(Error) as e:
    ...
assert '...' in str(e.value)
assert str(e.value) == '...'

We might want to test that the error message includes the correct column or incorrect value.

@llllllllll llllllllll added the testing label Feb 17, 2016

@@ -108,13 +108,21 @@ def test_join_option_types():
assert join(b, a, 'x').dshape == dshape('var * {x: int}')
def test_join_mismatched_schema():
def test_join_exceptions():
'mismatched schema; no shared fields'

This comment has been minimized.

@llllllllll

llllllllll Feb 17, 2016

Member

Can we use triple quoted strings for docstrings. It took me a second to realize this was the docstring.

This comment has been minimized.

@sandhujasmine

sandhujasmine Feb 17, 2016

Contributor

How do you feel about testing the contents of the error strings?

I feel good about it - I had added but wasn't sure if it was overkill. It's easy to add so I'll update.

Can we use triple quoted strings for docstrings

Yes, will fix.

This comment has been minimized.

@sandhujasmine

sandhujasmine Feb 17, 2016

Contributor

How do you feel about testing the contents of the error strings?
something like:

with pytest.raises(Error) as e:
   ...

@llllllllll

Instead of doing an exact match would it suffice to check substring? This will keep it concise and not require string formatting. I don't think exact match buys us much more once we've validated the correct exception was raised.

with pytest.raises(TypeError) as excinfo:
    join(a, b, 'x')

assert 'Schemata of joining columns do not match,' in str(excinfo.value)

The exact message for this exception is:
'Schemata of joining columns do not match,' ' no promotion found for %s=%s and %s=%s' % ( on_left[n], left_types[n], on_right[n], right_types[n], )

This comment has been minimized.

@llllllllll

llllllllll Feb 17, 2016

Member

that is fine, we might want to just check that the expected column names appear in the string

@llllllllll

This comment has been minimized.

Member

llllllllll commented Feb 17, 2016

Thank you for the test cases!

@sandhujasmine

This comment has been minimized.

Contributor

sandhujasmine commented Feb 17, 2016

@kwmsmith, @llllllllll

I was thinking to move exception tests for objects defined in collections module from test_interactive.py to test_collections.py, so moving:
test_sample_failures(), test_distinct_failures(), test_merge_failure()

I added the last two here but seems like the general convention is for tests for a module to be in associated test_.py

What do you think?

@llllllllll

This comment has been minimized.

Member

llllllllll commented Feb 17, 2016

Moving the tests sounds good. Keeping tests organized makes it easier for developers to find the place to add new tests which encourages better testing.

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Feb 18, 2016

I was thinking to move exception tests for objects defined in collections module from test_interactive.py to test_collections.py

👍

@llllllllll

This comment has been minimized.

Member

llllllllll commented Feb 19, 2016

Thank you for these tests and cleaning up the existing ones!

llllllllll added a commit that referenced this pull request Feb 19, 2016

@llllllllll llllllllll merged commit a96d889 into blaze:master Feb 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sandhujasmine sandhujasmine deleted the sandhujasmine:input-validation-tests branch Feb 19, 2016

kwmsmith added a commit to kwmsmith/blaze that referenced this pull request Feb 19, 2016

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