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

CI job to check that all test dirs are included in package #1054

Merged
merged 11 commits into from Aug 13, 2020

Conversation

freddyaboulton
Copy link
Contributor

@freddyaboulton freddyaboulton commented Aug 13, 2020

Pull Request Description

Closes #1031 . The new job should is called check_all_test_dirs_included. I tested it locally and by creating two dummy PRs and seeing if the CI job passes/fails as it should.

#1055 should fail the new check because the new test directory does not have an init file.
#1056 should pass the new check.


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.

@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #1054 into main will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1054   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files         187      188    +1     
  Lines       10278    10286    +8     
=======================================
+ Hits        10269    10277    +8     
  Misses          9        9           
Impacted Files Coverage Δ
evalml/tests/test_all_test_dirs_included.py 100.00% <100.00%> (ø)

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 0f08348...17708b5. Read the comment docs.

@freddyaboulton freddyaboulton marked this pull request as ready for review Aug 13, 2020
Copy link
Contributor

@jeremyliweishih jeremyliweishih left a comment

Looks good! After this you may need to go in the repo settings and make sure this test is required moving forward. You can find it here under required status checks.

Copy link
Contributor

@angela97lin angela97lin left a comment

LGTM! Curious (although I bet this is less likely for us to miss), is this necessary for the evalml source code folders too? :o

@freddyaboulton
Copy link
Contributor Author

freddyaboulton commented Aug 13, 2020

Thanks for the heads-up @jeremyliweishih ! I'll be sure to modify the settings.

Copy link
Collaborator

@dsherry dsherry left a comment

Thanks! CI config looks good. Let's use pytest to run the actual test though, yeah?

Makefile Outdated Show resolved Hide resolved
- install_dependencies_dev
- run: |
source test_python/bin/activate
make check-all-test-dirs-included
Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

Nice, circleci changes look good

.check_all_test_dirs_included Outdated Show resolved Hide resolved
test_dir = "evalml/tests"
all_test_dirs_with_init_files = [module for module in all_modules if "evalml.tests" in module]
all_test_dirs = [dirname for dirname, _, files in os.walk(test_dir) if "__pycache__" not in dirname and "test" in os.path.split(dirname)[1]]
assert len(all_test_dirs) == len(all_test_dirs_with_init_files), "Not every test directory in evalml/tests has an __init__.py file!"
Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

@freddyaboulton is there any reason you can't use pytest?

I also threw in pathlib.Path because why not make the pathing platform-agnostic 😆

from setuptools import find_packages
import os
from pathlib import Path
import pytest


def test_all_test_dirs_included():
    all_modules = find_packages()
    test_dir = Path("evalml", "tests")
    ... # etc

Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

Ah, and if you do this, I think there's no need for any changes to the CI! No need to add a new job. Because this unit test will run alongside all the others.

Makefile Outdated
@@ -43,6 +43,10 @@ installdeps-test:
installdeps-dev:
pip install -r dev-requirements.txt -q

.PHONY: check-all-test-dirs-included
check-all-test-dirs-included:
python .check_all_test_dirs_included.py
Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

I suggest we move this to evalml/tests/check_all_test_dirs_included.py

test_dir = os.path.dirname(__file__)
all_test_dirs_with_init_files = [module for module in all_modules if "evalml.tests" in module]
all_test_dirs = [dirname for dirname, _, files in os.walk(test_dir) if "__pycache__" not in dirname and "test" in os.path.split(dirname)[1]]
assert len(all_test_dirs) == len(all_test_dirs_with_init_files)
Copy link
Collaborator

@dsherry dsherry Aug 13, 2020

Choose a reason for hiding this comment

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

Sweet!

Copy link
Collaborator

@dsherry dsherry left a comment

🙏

@freddyaboulton
Copy link
Contributor Author

freddyaboulton commented Aug 13, 2020

@angela97lin That's a good question! I was hoping you wouldn't be able to import a file in a module that didn't have an init but that's not the case (#1057). I think since we haven't had issues with that problem before and since that's outside the scope of this issue I think it's ok as it is now?

@freddyaboulton
Copy link
Contributor Author

freddyaboulton commented Aug 13, 2020

@dsherry Thanks for the feedback. You're right, much simpler as a unit test!

@freddyaboulton freddyaboulton merged commit 3fe1630 into main Aug 13, 2020
@freddyaboulton freddyaboulton deleted the 1031-check-all-test-files-included-in-package branch Aug 13, 2020
@dsherry dsherry mentioned this pull request Aug 25, 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.

Add CI job to check all test files are included in package
4 participants