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

Fix tests utils to make --directory work correctly. #592

Merged
merged 7 commits into from
Sep 27, 2022

Conversation

robinwnv
Copy link
Contributor

Currently, when cwd is not cunumeric dir where test.py is located, if we use default root_dir or specify the directory, no test files would be found and zero division error is reported in summary.

In this change, we do two enhancements:

  1. In test_files method in config.py, use absolute path of test file which includes root_dir
  2. Fix total=0 in summary to avoid zero division error

@robinwnv robinwnv added the category:bug-fix PR is a bug fix and will be classified as such in release notes label Sep 20, 2022
@bryevdv
Copy link
Contributor

bryevdv commented Sep 20, 2022

@robinw0928 Your changes have broken the handling of SKIPPED_EXAMPLES:

SKIPPED_EXAMPLES = {
"examples/ingest.py",
"examples/kmeans_sort.py",
"examples/lstm_full.py",
"examples/wgrad.py",
}

There are four failing tests, exactly these four, that should not be run at all. You will need to update your changes to make sure that the SKIPPED_EXAMPLES set is still respected. The current handling is here, and you can see why it no longer works since the paths are absolute now:

examples = (
path
for path in Path("examples").glob("*.py")
if str(path) not in SKIPPED_EXAMPLES
)

Also, FYI I wrote tests for the test driver itself:

bldtest ❯ pytest tests/_utils/tests
================================ test session starts =================================
platform linux -- Python 3.9.13, pytest-7.1.2, pluggy-1.0.0
rootdir: /home/bryan/build_test/cunumeric, configfile: pyproject.toml
plugins: mock-3.8.2, cov-3.0.0, lazy-fixture-0.6.3
collected 153 items                                                                  

tests/_utils/tests/test___init__.py ...........                                [  7%]
tests/_utils/tests/test_args.py ........................                       [ 22%]
tests/_utils/tests/test_config.py ................................             [ 43%]
tests/_utils/tests/test_logger.py ........                                     [ 49%]
tests/_utils/tests/test_system.py .....                                        [ 52%]
tests/_utils/tests/test_types.py ..                                            [ 53%]
tests/_utils/tests/test_ui.py .............                                    [ 62%]
tests/_utils/tests/stages/test_test_stage.py ......                            [ 66%]
tests/_utils/tests/stages/test_util.py ..........                              [ 72%]
tests/_utils/tests/stages/_linux/test_cpu.py ............                      [ 80%]
tests/_utils/tests/stages/_linux/test_eager.py .......                         [ 84%]
tests/_utils/tests/stages/_linux/test_gpu.py .........                         [ 90%]
tests/_utils/tests/stages/_linux/test_omp.py ..............                    [100%]

================================ 153 passed in 1.49s =================================

They aren't yet run in CI. I plan to figure out how to add them to CI when the test driver code moves down to legate-core. I would appreciate if you try to make sure these test-driver tests are also updated for any changes you make. They can be run manually at the top level of cunumeric with pytest tests/_utils/tests

@robinwnv
Copy link
Contributor Author

Thanks @bryevdv
I choose to use relative path in test_file. In this way, we can support specifying --files using relative path.
"They can be run manually at the top level of cunumeric with pytest tests/_utils/tests" I have verified on my local machine. It pass.

@bryevdv
Copy link
Contributor

bryevdv commented Sep 21, 2022

@robinw0928 can you look in to installing the pre-commit hooks for this repo, so that the black formatting happens automatically? Alternatively, you can run black and isort manually before pushing, ot avoid all the pre-commit autofix noise in PRs.

tests/_utils/config.py Outdated Show resolved Hide resolved
tests/_utils/config.py Outdated Show resolved Hide resolved
tests/_utils/config.py Outdated Show resolved Hide resolved
@bryevdv
Copy link
Contributor

bryevdv commented Sep 23, 2022

@robinw0928 The now unused PurePath import needs to be removed. This is another good reason to make sure the pre-commit hooks are installed, this would have been caught locally, before you could push changes:

tests/_utils/config.py:22:1: F401 'pathlib.PurePath' imported but unused

@robinwnv robinwnv merged commit b4f52e6 into nv-legate:branch-22.10 Sep 27, 2022
@robinwnv robinwnv deleted the enhance_test_py branch March 14, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bug-fix PR is a bug fix and will be classified as such in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants