Skip to content

Refactors for component_class loops to use pytest.mark.parametrize#3622

Merged
chukarsten merged 6 commits into
alteryx:mainfrom
harshvardhanb25:main
Jul 27, 2022
Merged

Refactors for component_class loops to use pytest.mark.parametrize#3622
chukarsten merged 6 commits into
alteryx:mainfrom
harshvardhanb25:main

Conversation

@harshvardhanb25

Copy link
Copy Markdown
Contributor

Pull Request Description

Fixes: #3621

I have fixed most of the for loops to use @pytest.mark.parametrize and have also updated the if...continue cases to use @pytest.mark.xfail. One test case is currently failing but this was happening at the time of the initial fork.


After creating the pull request: in order to pass the release_notes_updated check you will need to update the "Future Release" section of docs/source/release_notes.rst to include this pull request by adding :pr:123.

@CLAassistant

CLAassistant commented Jul 21, 2022

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@harshvardhanb25 harshvardhanb25 changed the title Refactors for component_class loops to use pytest.mark.xfail Refactors for component_class loops to use pytest.mark.parametrize Jul 21, 2022
@harshvardhanb25 harshvardhanb25 changed the title Refactors for component_class loops to use pytest.mark.parametrize DRAFT Refactors for component_class loops to use pytest.mark.parametrize Jul 21, 2022
@codecov

codecov Bot commented Jul 22, 2022

Copy link
Copy Markdown

Codecov Report

Merging #3622 (7b61b90) into main (616496f) will not change coverage.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main   #3622   +/-   ##
=====================================
  Coverage   99.7%   99.7%           
=====================================
  Files        335     335           
  Lines      33508   33508           
=====================================
  Hits       33386   33386           
  Misses       122     122           
Impacted Files Coverage Δ
evalml/tests/component_tests/test_components.py 99.1% <100.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 616496f...7b61b90. Read the comment docs.

@jeremyliweishih

Copy link
Copy Markdown
Collaborator

hey @harshvardhanb25 - thanks for contributing! We need to manually approve your CI runs as you're a first time contributor but I just kicked them off. Please take a look at the failing CI and let me know if you have any questions on fixing them. Looks like you will need to update the release notes to pass the Release notes updated check. Once the CI is fix I'll give the PR a review!

@harshvardhanb25

harshvardhanb25 commented Jul 25, 2022

Copy link
Copy Markdown
Contributor Author

Hi @jeremyliweishih I have updated the release notes. However, when I run pylint on componet_tests.py, I get several messages, most of which have nothing to do with the changes I have made to the code. Could you please guide me on how to fix this?

@jeremyliweishih

Copy link
Copy Markdown
Collaborator

@harshvardhanb25 in the EvalML repo you can run make installdeps-dev to install developer dependencies and then pre-commit run to make the lint fixes (if you try to commit after installing developer dependencies it should lint for you as well). Not sure what messages you're getting but we do not use pylint for linting since we use black. Let me know if this helps!

@harshvardhanb25

Copy link
Copy Markdown
Contributor Author

@jeremyliweishih I think this should be it. I have updated the release notes and run the lint hooks. Thanks for your help!

@jeremyliweishih

Copy link
Copy Markdown
Collaborator

@harshvardhanb25 no worries! I will get to the review soon - thanks for helping out.

@harshvardhanb25 harshvardhanb25 changed the title DRAFT Refactors for component_class loops to use pytest.mark.parametrize Refactors for component_class loops to use pytest.mark.parametrize Jul 26, 2022

@jeremyliweishih jeremyliweishih left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@harshvardhanb25 great work on this! Just two suggestions:

  1. could you remove the test level @pytest.mark.xfail? I think it is redundant with the pytest.xfail() calls within the test.
  2. instead of the using the if statements to call pytest.xfail() could we just use the condition parameter so that we retain information on why these cases were allowed to fail.

Point 1 is the important one but take a shot at point 2 (I can approve once point 1 is fixed 😄 )! Let me know how I can help.

@harshvardhanb25

Copy link
Copy Markdown
Contributor Author

@jeremyliweishih I think pytest.xfail() does not take a condition parameter like @pytest.mark.xfail does. I tried doing this and the cases failed because of the additional parameter. Please let me know if there is something that might fix this. I am fixing the first issue you mentioned.

@jeremyliweishih

Copy link
Copy Markdown
Collaborator

@harshvardhanb25 I think you're correct! Thats my mistake 😄 will review once CI passes and will also get you another reviewer tmr. Thanks!

@harshvardhanb25

Copy link
Copy Markdown
Contributor Author

@jeremyliweishih thanks for all your help😄. Looking forward to contributing more to this repo.

@jeremyliweishih

Copy link
Copy Markdown
Collaborator

@harshvardhanb25 this PR LGTM, thanks for helping out!

@chukarsten chukarsten left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks! If you want more commits for your Github portfolio, I plan on putting out some more of these refactoring type issues!

@chukarsten chukarsten merged commit 7046113 into alteryx:main Jul 27, 2022
@harshvardhanb25

Copy link
Copy Markdown
Contributor Author

@chukarsten that would be awesome! I’m really looking to get my hands on some problems I can try fixing.

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.

Refactor test_components.py

4 participants