Skip to content

Check test exit codes.#37

Merged
vyasr merged 1 commit intorapidsai:branch-24.06from
bdice:check-test-exit
Apr 4, 2024
Merged

Check test exit codes.#37
vyasr merged 1 commit intorapidsai:branch-24.06from
bdice:check-test-exit

Conversation

@bdice
Copy link
Copy Markdown
Contributor

@bdice bdice commented Apr 3, 2024

Currently, the tests added in #27 (backported to 24.04 in #36) do not check the exit codes of their subprocesses. This means that failing tests are not caught. This PR fixes the test utilities to check the exit codes and print any stdout/stderr outputs.

@bdice bdice requested review from a team and vyasr as code owners April 3, 2024 21:30
@bdice bdice changed the base branch from branch-24.06 to branch-24.04 April 3, 2024 21:30
@bdice
Copy link
Copy Markdown
Contributor Author

bdice commented Apr 3, 2024

This PR stacks on top of #36 but we can separate it if needed. We'll need to decide if we are backporting in #36 or not before we can merge this.

from multiprocessing import Process


def run_test_in_subprocess(func):
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This file contains the relevant changes. See commit 60b8d7a.

Copy link
Copy Markdown
Member

@rjzamora rjzamora left a comment

Choose a reason for hiding this comment

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

@bdice - Thanks for putting this together!

I think it makes sense to target 24.06 with this change. That way, the testing improvement and #38 can be backported at once.

@bdice bdice changed the base branch from branch-24.04 to branch-24.06 April 4, 2024 14:41
@bdice bdice force-pushed the check-test-exit branch from 60b8d7a to f76cf2f Compare April 4, 2024 14:42
Copy link
Copy Markdown
Member

@rjzamora rjzamora left a comment

Choose a reason for hiding this comment

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

Thanks @bdice - Works for me locally when I remove the expected patch:

tests/test_patch.py::test_dask Process Process-1:
Traceback (most recent call last):
  File "/datasets/rzamora/miniconda3/envs/cudf_2406/lib/python3.11/multiprocessing/process.py", line 314, in _bootstrap
    self.run()
  File "/datasets/rzamora/miniconda3/envs/cudf_2406/lib/python3.11/multiprocessing/process.py", line 108, in run
    self._target(*self._args, **self._kwargs)
  File "/home/nfs/rzamora/workspace/cudf-24.06/rapids-dask-dependency/tests/test_patch.py", line 13, in redirect_stdout_stderr
    func(*args, **kwargs)
  File "/home/nfs/rzamora/workspace/cudf-24.06/rapids-dask-dependency/tests/test_patch.py", line 44, in test_dask
    assert hasattr(dask, "_rapids_patched")
AssertionError: assert False
 +  where False = hasattr(<module 'dask' (/datasets/rzamora/miniconda3/envs/cudf_2406/lib/python3.11/site-packages/dask/__init__.py)>, '_rapids_patched')
FAILED

@bdice
Copy link
Copy Markdown
Contributor Author

bdice commented Apr 4, 2024

@vyasr Feel free to review and/or merge when you're ready. Just want to avoid conflicts with #38 and let you control the merge timeline since you're doing other significant work here.

@vyasr vyasr merged commit 7726daa into rapidsai:branch-24.06 Apr 4, 2024
@vyasr
Copy link
Copy Markdown
Contributor

vyasr commented Apr 4, 2024

Thanks Bradley!

vyasr pushed a commit to vyasr/rapids-dask-dependency that referenced this pull request Apr 4, 2024
Currently, the tests added in rapidsai#27 (backported to 24.04 in rapidsai#36) do not
check the exit codes of their subprocesses. This means that failing
tests are not caught. This PR fixes the test utilities to check the exit
codes and print any stdout/stderr outputs.
raydouglass pushed a commit that referenced this pull request Apr 5, 2024
This PR backports #27, #37, and #39 to 24.04

---------

Signed-off-by: Vyas Ramasubramani <vyasr@nvidia.com>
Co-authored-by: Richard (Rick) Zamora <rzamora217@gmail.com>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
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.

3 participants