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

use mock_customer fixture #1036

Merged
merged 22 commits into from Jul 2, 2020
Merged

Conversation

systemshift
Copy link
Contributor

Pull Request Description

closes #1001
Unless I misunderstood, the issue has been mostly solved with the exception of this single case.
There are only 3 times ft.demo.load_mock_customer is called in tests, 2 already have fixtures.


After creating the pull request: in order to pass the changelog_updated check you will need to update the "Future Release" section of docs/source/changelog.rst to include this pull request.

@codecov
Copy link

codecov bot commented Jun 28, 2020

Codecov Report

No coverage uploaded for pull request base (main@45998e8). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1036   +/-   ##
=======================================
  Coverage        ?   98.35%           
=======================================
  Files           ?      126           
  Lines           ?    13036           
  Branches        ?        0           
=======================================
  Hits            ?    12821           
  Misses          ?      215           
  Partials        ?        0           

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 45998e8...3cef57a. Read the comment docs.

@thehomebrewnerd
Copy link
Collaborator

thehomebrewnerd commented Jun 29, 2020

This entityset is also used in test_demo_data.py on Line 26.

I think we should also move this fixture definition to conftest.py along with the Dask version, and parameterize them like we have done with the other fixtures. See test_calculate_feature_matrix.py Line 1458 for the fixture definitions. We should then make sure that there are no versions of this fixture defined in the individual tests files unless there is some specific need to do so.

@systemshift
Copy link
Contributor Author

systemshift commented Jun 29, 2020

@thehomebrewnerd please review and let me know if I should change anything.
I do not know what is causing the codecov issue, given that I haven't touched that file.

docs/source/changelog.rst Outdated Show resolved Hide resolved
featuretools/tests/demo_tests/test_demo_data.py Outdated Show resolved Hide resolved
featuretools/tests/utils_tests/test_entity_utils.py Outdated Show resolved Hide resolved
featuretools/tests/utils_tests/test_entity_utils.py Outdated Show resolved Hide resolved
@thehomebrewnerd
Copy link
Collaborator

@thehomebrewnerd please review and let me know if I should change anything.
I do not know what is causing the codecov issue, given that I haven't touched that file.

The codecov issue is likely because you are reducing the number of total lines of code, so the percent covered is decreasing.

@thehomebrewnerd
Copy link
Collaborator

@systemshift Please merge in the latest changes from featuretools/master and resolve the conflicts in changelog.rst.

docs/source/changelog.rst Outdated Show resolved Hide resolved
docs/source/changelog.rst Outdated Show resolved Hide resolved
thehomebrewnerd
thehomebrewnerd previously approved these changes Jul 2, 2020
Copy link
Collaborator

@thehomebrewnerd thehomebrewnerd left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the contribution!

@rwedge rwedge changed the base branch from master to main July 2, 2020 18:30
@rwedge rwedge dismissed thehomebrewnerd’s stale review July 2, 2020 18:30

The base branch was changed.

Copy link
Contributor

@rwedge rwedge left a comment

Choose a reason for hiding this comment

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

Looks good

@rwedge rwedge merged commit 8693e5c into alteryx:main Jul 2, 2020
@systemshift systemshift deleted the load_mock_customer_fixture branch July 23, 2020 03:07
@rwedge rwedge mentioned this pull request Jul 31, 2020
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.

Create a pytest fixture for load_mock_customer in tests
3 participants