CLN: Clean up of CI scripts #28482
CLN: Clean up of CI scripts #28482
Conversation
ci/run_tests.sh
Outdated
| echo "pandas could not detect the locale. System locale: $LOCALE_OVERRIDE, pandas detected: $PANDAS_LOCALE" | ||
| # TODO Not really aborting the tests until https://github.com/pandas-dev/pandas/issues/23923 is fixed | ||
| # exit 1 | ||
| # Testing if this is still failing | ||
| exit 1 |
There was a problem hiding this comment.
This is the part of the code that is making the CI fail, the rest is just styling.
| print("SKIPPED TESTS:") | ||
| i = 1 | ||
| for file_type in ("-single", "-multi", ""): | ||
| for test_data in main("test-data{}.xml".format(file_type)): |
There was a problem hiding this comment.
Minor stylistic comment but does using a glob with path lib and iterating over results simplify this? Took me a bit to grok what we were doing here and I think a glob would be more stable if we added xml docs or changed name in future
There was a problem hiding this comment.
Good point, didn't think about that. But in the next PR after this one I'll make the test just run once and remove the single/multi, so this loop will be removed. So not really worth to change it.
| if os.path.isfile(fn): | ||
| print(parse_results(fn)) | ||
| return 0 | ||
| yield None |
There was a problem hiding this comment.
Can you explain why this yields None? Does this not print skips that aren't associated to a class?
Haven't looked deeply but compared the number of skipped tests in this build to another from today for Linux Py35_Compat and numbers were way lower, so just curious if this is a reason for that
There was a problem hiding this comment.
This yield None shouldn't prevent the next yield from happening, so it's not causing to miss tests.
In the output of the skipped tests we print some division (i.e. --------------) between classes, so it's easier to see the different groups of tests that have been skipped.
With this yield None this becomes as simple as the if in the caller, and reduced significantly the code here, and makes things much compact and clear, which is the goal of this PR.
There was a problem hiding this comment.
There was indeed a typo that was causing the skipped tests of the multiple file to not be printed, good catch. Should be fixed now.
There was a problem hiding this comment.
Cool. Looks like this is printing out more skips now though?
There was a problem hiding this comment.
I don't think so. If you're checking the number of the last test, before this PR that is not the number of tests. The "single" and "multiple" tests were printed independently, so they start both from 1 and are not added up.
In this PR I'm fixing that, and the numbers don't reset. So, for example, in the Linux py35_compat, not it shows from 1 to 2,902. And before it was 1 to 568, and then 1 to 2334. The cases that I've checked, the number of the tests are the same, just the numbers in the second batch don't match.
Does this explain the difference you saw?
There was a problem hiding this comment.
Yea makes sense - thanks for clarifying
|
This should only affect the CI (and other than the small changes to the visualization of the skipped tests, there is no impact more than in the code clarity. So I don't think anything needs to be backported. |
Making the CI scripts simpler, and fixing typos (this was part of #26949, which couldn't be merged because
--dist=loadfilewas making the tests much slower).