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

Fixed #297 update tests to check error strings #303

Merged
merged 2 commits into from Nov 2, 2018

Conversation

jeff-hernandez
Copy link
Contributor

  • 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
Copy link

CLAassistant commented Oct 31, 2018

CLA assistant check
All committers have signed the CLA.

@kmax12
Copy link
Contributor

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=''):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jeff-hernandez
Copy link
Contributor Author

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

@kmax12
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

kmax12 commented Nov 2, 2018

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

@kmax12
Copy link
Contributor

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
Copy link
Contributor Author

Okay sounds good. I made the adjustment.

@kmax12
Copy link
Contributor

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
Copy link
Contributor Author

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

@kmax12
Copy link
Contributor

kmax12 commented Nov 2, 2018

Looks good. Merging. Thanks for the contribution!

@kmax12 kmax12 merged commit 7e57c70 into alteryx:master Nov 2, 2018
@rwedge rwedge mentioned this pull request Nov 29, 2018
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