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

GH-15839: move explain related tests to one stage #15844

Merged

Conversation

tomasfryda
Copy link
Contributor

@tomasfryda tomasfryda self-assigned this Oct 18, 2023
@tomasfryda tomasfryda added this to the 3.44.0.2 milestone Oct 18, 2023
The run.py when provided with a test list runs everything on the test
list so we need to exclude NOPASS from the testlist. When running the
other tests the list .lookup is used as exclude list and the NOPASS
tests are excluded while searching for the tests.
find h2o-r/tests -name '*explain*.R' -exec basename {} \; >>h2o-r/tests/.lookup.txt
find h2o-py/tests -name '*explain*.py' -exec basename {} \; >>h2o-py/tests/.lookup.txt
find h2o-r/tests/testdir_misc/explain -name '*.R' -exec basename {} \; | grep -v NOPASS >>h2o-r/tests/.lookup.txt
find h2o-py/tests/testdir_misc/explain -name '*.py' -exec basename {} \; | grep -v NOPASS >>h2o-py/tests/.lookup.txt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The .lookup.txt serves as test list in the run.py. When the test is in the test list we run it even if it is NOPASS. Since the SHAP_NOPASS can take a day to finish I skip it here.

When we run pyunit-small etc. the .lookup.txt serves as exclude list - run.py excludes those tests and then finds the remaining tests while skipping the NOPASS tests. (

h2o-3/scripts/run.py

Lines 1344 to 1347 in 1e4e8ca

if is_nopass and not nopass:
# skip all NOPASS tests for regular runs but still count the number of NOPASS tests
self.nopass_counter += 1
continue
)

@tomasfryda tomasfryda requested a review from mn-mikke October 19, 2023 12:03
mn-mikke
mn-mikke previously approved these changes Oct 19, 2023
Copy link
Collaborator

@mn-mikke mn-mikke left a comment

Choose a reason for hiding this comment

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

This is great, thank you!

@tomasfryda
Copy link
Contributor Author

Not so great, we apparently call lookup even when there are no tests (I think before we checkout the repo). Hopefully, I fixed that. I will ask you for review again once I'm sure everything is ok. Sorry for wasting your time @mn-mikke.

@tomasfryda tomasfryda force-pushed the tomf_gh-15839_move_explain_related_tests_to_one_stage branch from 58f6d53 to 7f35c6d Compare October 30, 2023 13:56
Copy link
Collaborator

@mn-mikke mn-mikke left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@tomasfryda tomasfryda merged commit b3dfc37 into rel-3.44.0 Nov 1, 2023
2 checks passed
@tomasfryda tomasfryda deleted the tomf_gh-15839_move_explain_related_tests_to_one_stage branch November 1, 2023 11:46
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.

2 participants