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] prevent Python tests from leaving behind files #6626

Merged
merged 10 commits into from
Sep 3, 2024

Conversation

vnherdeiro
Copy link
Contributor

@vnherdeiro vnherdeiro commented Aug 28, 2024

Contributes to #6361

Prevents the following files from being left behind when the Python tests are run:

  • data_dask.csv
  • Tree4.gv
  • Tree4.gv.pdf

The graphivz artefacts from tests/python_package_test/test_plotting.py now go to the pytest tmp_path and are deleted after test run.

@jameslamb jameslamb changed the title #6361 graphivz Tree4.gv Tree4.gv.pdf are no longer persisted [ci] prevent graphviz tests from leaving behind files Aug 28, 2024
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

I haven't tested yet if you've caught all the cases where this happens, but these fixes look like what I'd expect. Please see the suggestions I left, and the changes I made to the title and description.

PR titles become commit messages and therefore changelog entries in this repo, so they should describe the changes in a way that would be clear to someone reading the release notes. To see the conventions we follow here, compare these:

tests/python_package_test/test_plotting.py Outdated Show resolved Hide resolved
tests/python_package_test/test_plotting.py Outdated Show resolved Hide resolved
tests/python_package_test/test_plotting.py Outdated Show resolved Hide resolved
@vnherdeiro
Copy link
Contributor Author

Noted and adressed your comments @jameslamb thanks for the prompt review

@jameslamb
Copy link
Collaborator

Please update this branch to the latest master (to pull in recent CI fixes), and fix the issues reported at https://github.com/microsoft/LightGBM/actions/runs/10605651987/job/29394936451?pr=6626.

You can fix those by running the following from the root of the repo:

pip install pre-commit
pre-commit run --all-files

Copy link
Collaborator

@borchero borchero left a comment

Choose a reason for hiding this comment

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

Are there any other such occurrences as in #6627 and here? Maybe we can merge all of these changes into a single PR?

@vnherdeiro
Copy link
Contributor Author

@borchero According to #6361 these are the last two causes of test "leftover" files. I am okay with merging the two PRs.

Copy link
Collaborator

@borchero borchero left a comment

Choose a reason for hiding this comment

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

LGTM :)

@vnherdeiro
Copy link
Contributor Author

vnherdeiro commented Sep 2, 2024

Have now merged #6627 as the total code diff is small and they share the same purpose. I let the moderators close the other PR.

@borchero borchero changed the title [ci] prevent graphviz tests from leaving behind files [test] prevent Python tests from leaving behind files Sep 2, 2024
@borchero borchero linked an issue Sep 2, 2024 that may be closed by this pull request
10 tasks
@jameslamb jameslamb self-requested a review September 2, 2024 20:35
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Ok thanks very much! I've merged in latest master to get the changes from #6636, will merge this if CI passes.

We really appreciate the help!

@jameslamb jameslamb changed the title [test] prevent Python tests from leaving behind files [ci] prevent Python tests from leaving behind files Sep 2, 2024
@vnherdeiro
Copy link
Contributor Author

Thanks for the approvals. I am very pleased with this first minor contribution to a package I have been using with enjoyment and satisfaction! Let's see if I can add more to it in the future.

@jameslamb jameslamb merged commit d515039 into microsoft:master Sep 3, 2024
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ci] [python-package] Python tests leave files behind
3 participants