Show failed tests on CircleCI layout in a better way #25895

ydshieh · 2023-08-31T16:30:17Z

What does this PR do?

I am not sure if this will helps the navigation (much), but at least for the team members to try.

A new step is added to show the (only faiiled) test results, and it looks like

The Run tests step won't fail now (otherwise we can't have the new Check test results step), but the new step will (if there is indeed a failed test).

For a job run page, see here

WDYT?

ydshieh · 2023-08-31T16:31:28Z

.circleci/create_circleci_config.py

            test_command = f"({test_command}) || true"
        else:
-            test_command += " | tee tests_output.txt"
+            test_command += " | tee tests_output.txt || true"


This step will always succeed. This is not ideal, but it's necessary for the next step (Check test results) to run (when some tests fail).

ydshieh · 2023-08-31T16:32:00Z

.circleci/create_circleci_config.py

+        check_test_command = f'if [ -s reports/tests_{self.name}/failures_short.txt ]; '
+        check_test_command += 'then echo "Some test failed!"; echo ""; '
+        check_test_command += f'cat reports/tests_{self.name}/failures_short.txt; '
+        check_test_command += 'echo ""; echo ""; '
+
+        py_command = f'import os; fp = open("reports/tests_{self.name}/summary_short.txt"); failed = os.linesep.join([x for x in fp.read().split(os.linesep) if x.startswith("FAILED ")]); fp.close(); fp = open("summary_short.txt", "w"); fp.write(failed); fp.close()'
+        check_test_command += f"$(python3 -c '{py_command}'); "
+
+        check_test_command += f'cat summary_short.txt; exit -1; '
+        check_test_command += f'elif [ -s reports/tests_{self.name}/stats.txt ]; then echo "All tests pass!"; '
+
        # return code `124` means the previous (pytest run) step is timeout
        if self.name == "pr_documentation_tests":
-            checkout_doctest_command = 'if [ -s reports/tests_pr_documentation_tests/failures_short.txt ]; '
-            checkout_doctest_command += 'then echo "some test failed"; '
-            checkout_doctest_command += 'cat reports/tests_pr_documentation_tests/failures_short.txt; '
-            checkout_doctest_command += 'cat reports/tests_pr_documentation_tests/summary_short.txt; exit -1; '
-            checkout_doctest_command += 'elif [ -s reports/tests_pr_documentation_tests/stats.txt ]; then echo "All tests pass!"; '
-            checkout_doctest_command += 'elif [ -f 124.txt ]; then echo "doctest timeout!"; else echo "other fatal error)"; exit -1; fi;'
-            steps.append({"run": {"name": "Check doctest results", "command": checkout_doctest_command}})
+            check_test_command += 'elif [ -f 124.txt ]; then echo "doctest timeout!"; '
+
+        check_test_command += 'else echo "other fatal error"; exit -1; fi;'
+        check_test_command += 'echo ""; '
+
+        steps.append({"run": {"name": "Check test results", "command": check_test_command}})


A bit ugly, but just to show the failed (and only failed) tests.

So far I put failures_short.tx. We can use failures_long.tx if you prefer.

HuggingFaceDocBuilderDev · 2023-08-31T17:02:47Z

The documentation is not available anymore as the PR was closed or merged.

ArthurZucker

I'm not 100% sure there is a lot of difference between this and looking at the artifacts. Also if we do this, we could also remove the call to | tee following #23027.
This way you can either have a nice looking output on the page or go and check the artifacts.

ydshieh · 2023-09-04T12:45:21Z

No problem to close if you maintainers don't feel it makes navigation (even slightly) better/faster.

ArthurZucker · 2023-09-04T13:19:31Z

I'm actually in favor if we also remove tee 😉

amyeroberts

The output is so much better - thank you!

ydshieh · 2023-09-05T12:01:15Z

@amyeroberts BTW, the tests_output.txt will be removed to make @ArthurZucker happier. But want to have your confirmation too.

ydshieh · 2023-09-05T12:13:35Z

No more tea Sir, sorry.

ArthurZucker

Arf, it seems that we still don't have the nice output I was hoping for (with colors and more readable errors). Do you know why / could you try aiming for this?

ex:

ydshieh · 2023-09-05T13:04:12Z

I need a run (page) that already has such colors and format.

Personally, I don't feel it's necessary to have it - as we (mostly if not always) care the failing ones. The new step already give ONLY the failed tests without the passing and skipped one - so the color doesn't matter (at least for me).

I would move on to merge this PR as you and amy is OK with the changes. For the color thing, if you can provide a run page, I could probably give it a try in the future.

ArthurZucker · 2023-09-05T13:06:32Z

Sure 😉 feel free to merge!

ydshieh · 2023-09-05T13:21:49Z

We do have color (on the test run step, not the new test result showing step), but it only show the failing ones. See here.

On current main, no RED color at all.

amyeroberts · 2023-09-05T13:24:14Z

Happy for tests_output.txt to go - all the info can be found in the other artefacts

ArthurZucker · 2023-09-05T13:25:23Z

Perfect!

* update * update * fix --------- Co-authored-by: ydshieh <[email protected]>

update

9d0691c

ydshieh commented Aug 31, 2023

View reviewed changes

update

0dc5845

ydshieh requested review from ArthurZucker and amyeroberts August 31, 2023 16:43

ydshieh requested review from ArthurZucker and amyeroberts and removed request for ArthurZucker and amyeroberts September 4, 2023 11:17

ArthurZucker reviewed Sep 4, 2023

View reviewed changes

fix

dd80e72

amyeroberts approved these changes Sep 5, 2023

View reviewed changes

ydshieh requested a review from ArthurZucker September 5, 2023 12:13

ArthurZucker reviewed Sep 5, 2023

View reviewed changes

ydshieh merged commit aa5c94d into main Sep 5, 2023

ydshieh deleted the show_failure_better branch September 5, 2023 13:49

ArthurZucker mentioned this pull request Sep 5, 2023

[CI] Fix red CI and ERROR failed should show #25995

Merged

ydshieh mentioned this pull request Sep 7, 2023

Fix CircleCI config #26023

Merged

parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023

Show failed tests on CircleCI layout in a better way (huggingface#25895)

6c3a1dd

* update * update * fix --------- Co-authored-by: ydshieh <[email protected]>

Show failed tests on CircleCI layout in a better way #25895

Show failed tests on CircleCI layout in a better way #25895

Uh oh!

Conversation

ydshieh commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

ydshieh Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

ydshieh Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

ydshieh Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

HuggingFaceDocBuilderDev commented Aug 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArthurZucker left a comment

Choose a reason for hiding this comment

Uh oh!

ydshieh commented Sep 4, 2023

Uh oh!

ArthurZucker commented Sep 4, 2023

Uh oh!

amyeroberts left a comment

Choose a reason for hiding this comment

Uh oh!

ydshieh commented Sep 5, 2023

Uh oh!

ydshieh commented Sep 5, 2023

Uh oh!

ArthurZucker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydshieh commented Sep 5, 2023

Uh oh!

ArthurZucker commented Sep 5, 2023

Uh oh!

ydshieh commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amyeroberts commented Sep 5, 2023

Uh oh!

ArthurZucker commented Sep 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ydshieh commented Aug 31, 2023 •

edited

Loading

HuggingFaceDocBuilderDev commented Aug 31, 2023 •

edited

Loading

ArthurZucker left a comment •

edited

Loading

ydshieh commented Sep 5, 2023 •

edited

Loading