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

Add tests for Entity methods #262

Merged
merged 4 commits into from Sep 20, 2018
Merged

Add tests for Entity methods #262

merged 4 commits into from Sep 20, 2018

Conversation

Seth-Rothschild
Copy link
Contributor

This PR adds three tests and fixes the add_variable method for Entity.

  • Fixes a faulty assertion and expected column types in entity.add_variable
  • Test entity.add_variable
  • Adds tests for two value errors in entity.update_data
  • Test the previously untested elif unit == 'Y' block in add_td

@codecov-io
Copy link

codecov-io commented Sep 19, 2018

Codecov Report

Merging #262 into master will increase coverage by 0.29%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #262      +/-   ##
==========================================
+ Coverage   94.14%   94.43%   +0.29%     
==========================================
  Files          71       71              
  Lines        7632     7639       +7     
==========================================
+ Hits         7185     7214      +29     
+ Misses        447      425      -22
Impacted Files Coverage Δ
featuretools/entityset/entity.py 93.49% <ø> (+3.71%) ⬆️
featuretools/tests/entityset_tests/test_entity.py 100% <100%> (ø) ⬆️
...aturetools/tests/entityset_tests/test_timedelta.py 100% <100%> (ø) ⬆️
featuretools/entityset/timedelta.py 86.42% <0%> (+5.71%) ⬆️

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 33c357b...9e489cd. Read the comment docs.

assert 'Updated dataframe contains 13 columns, expecting 12' in str(excinfo)


def test_add_variable(es):
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than add a test for add_variable, let's just remove the function. it's only used in one place internally and we can probably avoid needing it at all

@Seth-Rothschild
Copy link
Contributor Author

It seems it's not possible to satisfy the conditional

if self.index is not None and self.index not in inferred_variable_types

which used add_variables. Either you set the index as the index of the entity and self.infer_variable_types will catch it or you set the variable type as variable_types.Index, in which case it will be automatically included in the inferred_variable_types dictionary.

If there is a situation which can happen, it would be easy to add the relevant lines from add_variable under this conditional.

@kmax12
Copy link
Contributor

kmax12 commented Sep 20, 2018

@Seth-Rothschild ya, that makes sense to me. let's remove that code block and the add_variables function.

edit: looks like you already did.

@kmax12
Copy link
Contributor

kmax12 commented Sep 20, 2018

Looks good. Merging

@kmax12 kmax12 merged commit 633f901 into master Sep 20, 2018
@Seth-Rothschild Seth-Rothschild deleted the add_es_tests branch September 20, 2018 19:33
@kmax12 kmax12 mentioned this pull request Sep 28, 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