Skip to content

MAINT Shorten parametrized test names with long error messages #21364

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 19, 2021
Merged

MAINT Shorten parametrized test names with long error messages #21364

merged 3 commits into from
Oct 19, 2021

Conversation

ogrisel
Copy link
Member

@ogrisel ogrisel commented Oct 18, 2021 •
edited
Loading

Fixes #21362.

I think those long error messages are too long anyways.

@ogrisel
Copy link
Member Author

ogrisel commented Oct 18, 2021

Here is the output of pytest for the following command:

pytest -v  -k test_max_samples_exceptions sklearn/ensemble

sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[1000000000-ValueError-error-ExtraTreesClassifier] PASSED                           [  3%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[1000000000-ValueError-error-RandomForestClassifier] PASSED                         [  7%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[1000000000-ValueError-error-ExtraTreesRegressor] PASSED                            [ 10%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[1000000000-ValueError-error-RandomForestRegressor] PASSED                          [ 14%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[2.0-ValueError-error-ExtraTreesClassifier] PASSED                                  [ 17%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[2.0-ValueError-error-RandomForestClassifier] PASSED                                [ 21%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[2.0-ValueError-error-ExtraTreesRegressor] PASSED                                   [ 25%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[2.0-ValueError-error-RandomForestRegressor] PASSED                                 [ 28%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[0.0-ValueError-error-ExtraTreesClassifier] PASSED                                  [ 32%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[0.0-ValueError-error-RandomForestClassifier] PASSED                                [ 35%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[0.0-ValueError-error-ExtraTreesRegressor] PASSED                                   [ 39%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[0.0-ValueError-error-RandomForestRegressor] PASSED                                 [ 42%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[nan-ValueError-error-ExtraTreesClassifier] PASSED                                  [ 46%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[nan-ValueError-error-RandomForestClassifier] PASSED                                [ 50%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[nan-ValueError-error-ExtraTreesRegressor] PASSED                                   [ 53%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[nan-ValueError-error-RandomForestRegressor] PASSED                                 [ 57%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[inf-ValueError-error-ExtraTreesClassifier] PASSED                                  [ 60%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[inf-ValueError-error-RandomForestClassifier] PASSED                                [ 64%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[inf-ValueError-error-ExtraTreesRegressor] PASSED                                   [ 67%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[inf-ValueError-error-RandomForestRegressor] PASSED                                 [ 71%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[error-TypeError-error-ExtraTreesClassifier] PASSED                                 [ 75%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[error-TypeError-error-RandomForestClassifier] PASSED                               [ 78%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[error-TypeError-error-ExtraTreesRegressor] PASSED                                  [ 82%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[error-TypeError-error-RandomForestRegressor] PASSED                                [ 85%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[max_samples6-TypeError-error-ExtraTreesClassifier] PASSED                          [ 89%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[max_samples6-TypeError-error-RandomForestClassifier] PASSED                        [ 92%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[max_samples6-TypeError-error-ExtraTreesRegressor] PASSED                           [ 96%]
sklearn/ensemble/tests/test_forest.py::test_max_samples_exceptions[max_samples6-TypeError-error-RandomForestRegressor] PASSED                         [100%]

jeremiedbb
jeremiedbb approved these changes Oct 18, 2021
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM

thomasjpfan
thomasjpfan reviewed Oct 18, 2021
View reviewed changes
sklearn/ensemble/tests/test_forest.py Outdated
@@ -1654,6 +1654,9 @@ def test_forest_degenerate_feature_importances():
r"'\<class 'numpy.ndarray'\>'",
),
],
# Avoid long error messages in test names:
# https://github.com/scikit-learn/scikit-learn/issues/21362
ids=lambda x: "error" if isinstance(x, str) and len(x) > 10 else x,
Copy link
Member

Choose a reason for hiding this comment

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

After some testing, I think it's the inclusion of ] that is causing that error.

If an exc_msg were to have len(x) <=10 and include a ], vscode would still fail to parse the collected test. If we want to have shorter test names and avoid the error:

Suggested change
ids=lambda x: "error" if isinstance(x, str) and len(x) > 10 else x,
ids=lambda x: x[:10].replace("]", "") if isinstance(x, str) else x,

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I agree about ] but I also think they will fix the problem upstream in VS Code (because it used to work, it's a recent regression).

So my idea was to make a fix to temporarily hide the VS Code bug and at the same time shorten those test names that are too verbose.

But ok with your suggestion. I think we can drop the .replace("]", "") once the bug is fixed in VS Code.

@ogrisel @thomasjpfan
Keep a safe fragment of the original error message
779e4d9 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
thomasjpfan
thomasjpfan approved these changes Oct 19, 2021
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

I am okay with this temporary fix and revert when microsoft/vscode-python#17676 is fixed.

@jeremiedbb
Copy link
Member

Still LGTM to me. I'm ok to revert the "replace" stuff when vscode is fixed but I'd like to keep the short id because it makes the pytest rendering more readable

@TomDLT TomDLT merged commit bd8528d into scikit-learn:main Oct 19, 2021
@ogrisel ogrisel deleted the pytest-shorter-test-names branch October 20, 2021 11:55
@glemaitre glemaitre mentioned this pull request Oct 23, 2021
Merged
10 tasks
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Oct 23, 2021
@ogrisel @thomasjpfan @glemaitre
MAINT Shorten parametrized test names with long error messages (sciki…
56a4b95 
…t-learn#21364)

* Apply suggestions from code review

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
glemaitre pushed a commit that referenced this pull request Oct 25, 2021
@ogrisel @thomasjpfan @glemaitre
MAINT Shorten parametrized test names with long error messages (#21364)
c405a00 
* Apply suggestions from code review

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@ogrisel @thomasjpfan @samronsin
MAINT Shorten parametrized test names with long error messages (sciki…
50078b3 
…t-learn#21364)

* Apply suggestions from code review

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:ensemble module:linear_model No Changelog Needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

scikit-learn test names break VS Code test discovery pluggin
4 participants
@ogrisel @jeremiedbb @thomasjpfan @TomDLT