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

Update CI to run doctest (docstring tests) and apply necessary fixes to docstrings #2933

Merged
merged 7 commits into from
Oct 19, 2021

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented Oct 18, 2021

Options:

  • Add separate job for doctests. Seems like overkill for now, given that we only have a handful? Could be good to revisit during Add more doctests to class modules and methods #2936
  • Currently this PR just adds to the git-test-others. If we expand the suite, could be worth to add to each individual git-test command as we have for pytest. :)

@codecov
Copy link

codecov bot commented Oct 18, 2021

Codecov Report

Merging #2933 (65ee645) into main (53242f9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2933   +/-   ##
=====================================
  Coverage   99.7%   99.7%           
=====================================
  Files        302     302           
  Lines      28433   28433           
=====================================
  Hits       28340   28340           
  Misses        93      93           
Impacted Files Coverage Δ
evalml/data_checks/datetime_format_data_check.py 100.0% <ø> (ø)
evalml/data_checks/outliers_data_check.py 100.0% <ø> (ø)
...alml/data_checks/target_distribution_data_check.py 100.0% <ø> (ø)
evalml/pipelines/component_graph.py 99.8% <ø> (ø)

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 53242f9...65ee645. Read the comment docs.

@angela97lin angela97lin changed the title [Spike] Test docstrings Update CI to run doctest (docstring tests) and apply necessary fixes to docstrings Oct 18, 2021
@@ -31,39 +31,41 @@ test-no-parallel:

.PHONY: test-parallel
test-parallel:
pytest evalml/tests/automl_tests/parallel_tests/ --doctest-modules --doctest-continue-on-failure --timeout 300 --durations 0
pytest evalml/tests/automl_tests/parallel_tests/ --timeout 300 --durations 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not useful to have the --doctest-modules tag in these commands, since there are no docstrings in our tests that we want to test :)

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

Good catch! Looks good to me @angela97lin !


.PHONY: git-test-other
git-test-other:
pytest evalml/tests --ignore evalml/tests/automl_tests/ --ignore evalml/tests/tuner_tests/ --ignore evalml/tests/model_understanding_tests/ --ignore evalml/tests/pipeline_tests/ --ignore evalml/tests/utils_tests/ --ignore evalml/tests/component_tests/test_prophet_regressor.py --ignore evalml/tests/component_tests/test_components.py --ignore evalml/tests/component_tests/test_utils.py -n 2 --durations 0 --timeout 300 --doctest-modules --cov=evalml --junitxml=test-reports/git-test-other-junit.xml --doctest-continue-on-failure
pytest evalml/tests --ignore evalml/tests/automl_tests/ --ignore evalml/tests/tuner_tests/ --ignore evalml/tests/model_understanding_tests/ --ignore evalml/tests/pipeline_tests/ --ignore evalml/tests/utils_tests/ --ignore evalml/tests/component_tests/test_prophet_regressor.py --ignore evalml/tests/component_tests/test_components.py --ignore evalml/tests/component_tests/test_utils.py -n 2 --durations 0 --timeout 300 --doctest-modules --cov=evalml --junitxml=test-reports/git-test-other-junit.xml
make doctests
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok to run as part of git-test-other!

nit-pick: Maybe we should make a note of it somewhere? So that we remember and new devs/users know what's happening. Not sure if the right place is contributing guidelines. We also have our own internal CI document?

@angela97lin
Copy link
Contributor Author

Great idea @freddyaboulton! Updated internal docs, and will add a brief line in our contributing guidelines to add doctests if applicable.

@angela97lin angela97lin merged commit 2767e33 into main Oct 19, 2021
@angela97lin angela97lin deleted the ange_test_docstring_asserts branch October 19, 2021 02:52
@chukarsten chukarsten mentioned this pull request Oct 27, 2021
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.

2 participants