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

Fixed #297 update tests to check error strings #303

Merged
merged 2 commits into from Nov 2, 2018

Conversation

Projects
None yet
3 participants
@jeff-hernandez
Collaborator

jeff-hernandez commented Oct 31, 2018

  • On windows platform, there is an open issue currently in pandas where it raises an error when reading a file with accents in the file path (i.e. régions.csv). So, I resolved it with the following:
# featuretools\tests\testing_utils\mock_ds.py:334
df = pd.read_csv(open(filenames[entity], 'r', encoding='utf8'), encoding='utf-8')
  • This snippet np.dtype((np.integer, np.floating)).type was causing this issue. So, I resolved it by changing it to the following:
np.issubdtype(time, np.integer) or np.issubdtype(time, np.floating)
  • Not sure how to get the error text for test_not_enough_memory
@CLAassistant

This comment has been minimized.

CLAassistant commented Oct 31, 2018

CLA assistant check
All committers have signed the CLA.

@kmax12

This comment has been minimized.

Member

kmax12 commented Oct 31, 2018

thanks @jeff-hernandez. circleci is having problems sending the status back to github right now, but looks like one test is failing: https://circleci.com/gh/Featuretools/featuretools/1418.

the error is AssertionError: Pattern 'cutoff_time times must be datetime type.*try casting via pd.to_datetime.*' not found in 'cutoff_time times must be numeric: try casting via pd.to_numeric(cutoff_time['time'])'

@@ -61,9 +62,9 @@ def test_delta_with_observations(es):
large_delta = Timedelta(99999, 'observations', 'log')('customers',
instance_id=0,
entityset=es)
with pytest.raises(NotEnoughData):
with pytest.raises(NotEnoughData, match=''):

This comment has been minimized.

@kmax12

kmax12 Oct 31, 2018

Member

no need to have match text here. In this case NotEnoughData is already specific enough

@jeff-hernandez jeff-hernandez force-pushed the jeff-hernandez:master branch from 34fa316 to dc6222a Oct 31, 2018

@jeff-hernandez

This comment has been minimized.

Collaborator

jeff-hernandez commented Oct 31, 2018

Thanks @kmax12. I updated the commit. Let me know if anything else needs to be changed.

@kmax12

This comment has been minimized.

Member

kmax12 commented Oct 31, 2018

@jeff-hernandez looks like python 2.7 tests are failing because encoding isn't a valid parameter to open in python 2.7.

@codecov

This comment has been minimized.

codecov bot commented Oct 31, 2018

Codecov Report

Merging #303 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #303      +/-   ##
==========================================
+ Coverage   95.03%   95.06%   +0.02%     
==========================================
  Files          71       71              
  Lines        7620     7660      +40     
==========================================
+ Hits         7242     7282      +40     
  Misses        378      378
Impacted Files Coverage Δ
featuretools/tests/testing_utils/mock_ds.py 87.4% <ø> (ø) ⬆️
...uretools/tests/entityset_tests/test_es_metadata.py 96.77% <100%> (+0.07%) ⬆️
...aturetools/tests/entityset_tests/test_timedelta.py 100% <100%> (ø) ⬆️
featuretools/tests/entityset_tests/test_es.py 99.37% <100%> (+0.01%) ⬆️
featuretools/tests/entityset_tests/test_entity.py 100% <100%> (ø) ⬆️
...utational_backend/test_calculate_feature_matrix.py 99.3% <100%> (+0.01%) ⬆️
...ols/tests/feature_function_tests/test_agg_feats.py 98.56% <100%> (+0.01%) ⬆️
featuretools/utils/wrangle.py 79.77% <100%> (ø) ⬆️
...ests/computational_backend/test_encode_features.py 100% <100%> (ø) ⬆️
.../feature_function_tests/test_transform_features.py 98.85% <100%> (ø) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 449df54...a027e19. Read the comment docs.

@jeff-hernandez jeff-hernandez force-pushed the jeff-hernandez:master branch 4 times, most recently from fca7ada to fe5c50c Nov 1, 2018

@jeff-hernandez

This comment has been minimized.

Collaborator

jeff-hernandez commented Nov 1, 2018

I switched back to the previous snippet:

df = pd.read_csv(filenames[entity], encoding='utf-8')

I still get the error for reading a file with accents in the file path. I couldn't get the error text to work in the function test_add_relationship_errors_on_dtype_mismatch because of unicode related issues.

@kmax12

This comment has been minimized.

Member

kmax12 commented Nov 1, 2018

does the following work for test_add_relationship_errors_on_dtype_mismatch to handle unicode?

error_text = u'Unable to add relationship because id in régions is Pandas dtype object and session_id in log2 is Pandas dtype int64.'
with pytest.raises(ValueError, match=error_text):
    mismatch = Relationship(entityset[u'régions']['id'], entityset['log2']['session_id'])
    entityset.add_relationship(mismatch)

other than that, it's good to merge!

@jeff-hernandez jeff-hernandez force-pushed the jeff-hernandez:master branch 2 times, most recently from 00a3898 to 43e3629 Nov 2, 2018

@jeff-hernandez

This comment has been minimized.

Collaborator

jeff-hernandez commented Nov 2, 2018

@kmax12 I tried the code snippet. This is the error I get for python 2.7:

UnicodeEncodeError: 'ascii' codec can't encode character u'\xe9' in position 42: ordinal not in range(128)
@kmax12

This comment has been minimized.

Member

kmax12 commented Nov 2, 2018

@jeff-hernandez im looking into this now. weirdly it's only a problem in python 2.7.

@kmax12

This comment has been minimized.

Member

kmax12 commented Nov 2, 2018

actually, here's an easier fix. let's just change the entity for this test case. I think that this solution is fine since we aren't trying to test unicode stuff with this test case.

So, it can just be

error_text = u'Unable to add relationship because id in customers is Pandas dtype category and session_id in log2 is Pandas dtype int64.'
with pytest.raises(ValueError, match=error_text):
    mismatch = Relationship(entityset[u'customers']['id'], entityset['log2']['session_id'])
    entityset.add_relationship(mismatch)

@jeff-hernandez jeff-hernandez force-pushed the jeff-hernandez:master branch 2 times, most recently from 416eb5b to 0992f03 Nov 2, 2018

@jeff-hernandez

This comment has been minimized.

Collaborator

jeff-hernandez commented Nov 2, 2018

Okay sounds good. I made the adjustment.

@kmax12

This comment has been minimized.

Member

kmax12 commented Nov 2, 2018

this is look good. actually one more stylistic change. In many places, you removed a blank line before the error test. can you add those back in?

for example

screen shot 2018-11-02 at 1 00 14 pm

after that, we're good to merge

@jeff-hernandez jeff-hernandez force-pushed the jeff-hernandez:master branch from 0897180 to 4aa02e5 Nov 2, 2018

@jeff-hernandez

This comment has been minimized.

Collaborator

jeff-hernandez commented Nov 2, 2018

No problem. I updated the adjustment. Let me know if anything else needs modification. Thanks.

@kmax12

This comment has been minimized.

Member

kmax12 commented Nov 2, 2018

Looks good. Merging. Thanks for the contribution!

@kmax12 kmax12 merged commit 7e57c70 into Featuretools:master Nov 2, 2018

3 checks passed

codecov/patch 100% of diff hit (target 95.03%)
Details
codecov/project 95.06% (+0.02%) compared to 449df54
Details
license/cla Contributor License Agreement is signed.
Details

@rwedge rwedge referenced this pull request Nov 29, 2018

Merged

v0.4.1 #329

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