Skip to content

CLN: Clean up of CI scripts #28482

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

Merged
merged 10 commits into from
Sep 19, 2019
Merged

CLN: Clean up of CI scripts #28482

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
61e503a
Reformat azure posix file
Sep 17, 2019
9d800c6
Adding failing test, simplifying print skipped and run tests
Sep 17, 2019
fb826c0
Testing if locale check is still broken
Sep 17, 2019
79fde27
Black
Sep 17, 2019
456ca3c
Removing failing test that tested that the CI was capturing errors pr…
Sep 17, 2019
c001857
Disabling locale test again, to pass CI in this PR
Sep 18, 2019
89491b9
Merge remote-tracking branch 'upstream/master' into ci_clean
Sep 18, 2019
571e205
Restoring proxy settings (is this making tests run much slower?)
Sep 18, 2019
084a475
Merge remote-tracking branch 'upstream/master' into ci_clean
datapythonista Sep 18, 2019
b174abd
Fixed typo in file name
datapythonista Sep 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ci/azure/posix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,15 +60,21 @@ jobs:
echo "Creating Environment"
ci/setup_env.sh
displayName: 'Setup environment and build pandas'

- script: |
source activate pandas-dev
ci/run_tests.sh
displayName: 'Test'

- script: source activate pandas-dev && pushd /tmp && python -c "import pandas; pandas.show_versions();" && popd
displayName: 'Build versions'

- task: PublishTestResults@2
inputs:
testResultsFiles: 'test-data-*.xml'
testRunTitle: ${{ format('{0}-$(CONDA_PY)', parameters.name) }}
displayName: 'Publish test results'

- powershell: |
$junitXml = "test-data-single.xml"
$(Get-Content $junitXml | Out-String) -match 'failures="(.*?)"'
Expand All @@ -94,6 +100,7 @@ jobs:
Write-Error "$($matches[1]) tests failed"
}
displayName: 'Check for test failures'

- script: |
source activate pandas-dev
python ci/print_skipped.py
Expand Down
58 changes: 23 additions & 35 deletions ci/print_skipped.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,52 +1,40 @@
#!/usr/bin/env python

import math
import os
import sys
import xml.etree.ElementTree as et


def parse_results(filename):
def main(filename):
if not os.path.isfile(filename):
return

tree = et.parse(filename)
root = tree.getroot()
skipped = []

current_class = ""
i = 1
assert i - 1 == len(skipped)
for el in root.findall("testcase"):
cn = el.attrib["classname"]
for sk in el.findall("skipped"):
old_class = current_class
current_class = cn
name = "{classname}.{name}".format(
classname=current_class, name=el.attrib["name"]
)
msg = sk.attrib["message"]
out = ""
if old_class != current_class:
ndigits = int(math.log(i, 10) + 1)

# 4 for : + space + # + space
out += "-" * (len(name + msg) + 4 + ndigits) + "\n"
out += "#{i} {name}: {msg}".format(i=i, name=name, msg=msg)
skipped.append(out)
i += 1
assert i - 1 == len(skipped)
assert i - 1 == len(skipped)
# assert len(skipped) == int(root.attrib['skip'])
return "\n".join(skipped)


def main():
test_files = ["test-data-single.xml", "test-data-multiple.xml", "test-data.xml"]

print("SKIPPED TESTS:")
for fn in test_files:
if os.path.isfile(fn):
print(parse_results(fn))
return 0
yield None
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. Looks like this is printing out more skips now though?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yea makes sense - thanks for clarifying

yield {
"class_name": current_class,
"test_name": el.attrib["name"],
"message": sk.attrib["message"],
}


if __name__ == "__main__":
sys.exit(main())
print("SKIPPED TESTS:")
i = 1
for file_type in ("-single", "-multi", ""):
for test_data in main("test-data{}.xml".format(file_type)):
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 test_data is None:
print("-" * 80)
else:
print(
"#{i} {class_name}.{test_name}: {message}".format(
**dict(test_data, i=i)
)
)
i += 1
13 changes: 3 additions & 10 deletions ci/run_tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,6 @@
#!/bin/bash
#!/bin/bash -e

set -e

if [ "$DOC" ]; then
echo "We are not running pytest as this is a doc-build"
exit 0
fi

# Workaround for pytest-xdist flaky collection order
# Workaround for pytest-xdist (it collects different tests in the workers if PYTHONHASHSEED is not set)
# https://github.com/pytest-dev/pytest/issues/920
# https://github.com/pytest-dev/pytest/issues/1075
export PYTHONHASHSEED=$(python -c 'import random; print(random.randint(1, 4294967295))')
Expand All @@ -16,7 +9,7 @@ if [ -n "$LOCALE_OVERRIDE" ]; then
export LC_ALL="$LOCALE_OVERRIDE"
export LANG="$LOCALE_OVERRIDE"
PANDAS_LOCALE=`python -c 'import pandas; pandas.get_option("display.encoding")'`
if [[ "$LOCALE_OVERIDE" != "$PANDAS_LOCALE" ]]; then
if [[ "$LOCALE_OVERRIDE" != "$PANDAS_LOCALE" ]]; then
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
Expand Down