8000 Unittester failures summary by hmeriann · Pull Request #16833 · duckdb/duckdb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Unittester failures summary #16833

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 77 commits into from
Jun 17, 2025
Merged

Unittester failures summary #16833

merged 77 commits into from
Jun 17, 2025

Conversation

hmeriann
Copy link
Contributor

Currently when we have a failure in the CI (running unit tests), the actual test failure is hidden in mountains of successful runs.

We should print the output of all failing tests again at the end of running the unittester so we can more easily find the failing tests. This can make it easier to look through the CI.

This PR adds the Failures Summary printed out right in the end of the logs by default.
Passing --summarize-failures flag to the ./build/release/test/unittest should enable summary output in the end of test run. There is a python script to run unit tests one by one (scripts/run_tests_one_by_one.py) which should print summary by default.

Python script captures the whole failure details message easily, but the unittest - only the failing case with the problematic line number.

Should look like this for unittest:

=====================================================
================  FAILURES  SUMMARY  ================
=====================================================

1: test/sql/aggregate/aggregates/histogram_table_function.test: 20 
2: test/sql/copy/csv/test_date.test: 11

And for scripts/run_tests_one_by_one.py:

=====================================================
================  FAILURES  SUMMARY  ================
=====================================================

1: ['test/sql/aggregate/aggregates/histogram_table_function.test']
FAILED WITH ERROR:
 ================================================================================
Wrong result in query! (test/sql/aggregate/aggregates/histogram_table_function.test:20)!
================================================================================
SELECT * FROM histogram_values(integers, i, bin_count := 2);
================================================================================
Mismatch on row 1, column count(index 2)
1 <> 1q
================================================================================
Expected result:
================================================================================
60      1q
80      0
100     1
================================================================================
Actual result:
================================================================================
60      1
80      0
100     1

2: ['test/sql/copy/csv/test_date.test']
FAILED WITH ERROR:
 ================================================================================
Wrong result in query! (test/sql/copy/csv/test_date.test:11)!
================================================================================
COPY date_test FROM 'data/csv/test/date.csv';
================================================================================
Mismatch on row 1, column Count(index 1)
1 <> 1q
================================================================================
Expected result:
================================================================================
1q
================================================================================
Actual result:
================================================================================
1

@hmeriann hmeriann requested a review from Tmonster March 25, 2025 20:14
@hmeriann hmeriann changed the title Unittester summary Unittester failures summary Mar 26, 2025
@Mytherin
Copy link
Collaborator

Thanks for the PR! LGTM - the CI failure is unrelated.

Can we start enabling these flags on the various CI runs as well?

Tmonster
Tmonster reviewed Mar 26, 2025
View reviewed changes
Copy link
Contributor
@Tmonster Tmonster left a comment

Choose a reason for hiding this comment

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

Looks good! Just the one question

@duckdb-draftbot duckdb-draftbot marked this pull request as draft March 26, 2025 14:13
@hmeriann
Copy link
Contributor Author

Made the summary look the same as summary of Python runner results - for now it's only for CompareValues(). Will add more

@hmeriann
Copy link
Contributor Author

I need to add maybe some test cases to test that all the failures will get into a summary

@hmeriann hmeriann marked this pull request as ready for review March 26, 2025 16:52
@hmeriann
Copy link
Contributor Author

there is the SQLLogicTestLogger which is used to output all the unittest test results. I've added a SQLLogicTestLogger::AddToSummary(string log_message) method to write found failure into a file.

I've also modified the return types of most of the SQLLogicTestLogger methods so that the strings they return can be collected into a log_message variable.

I think that the filename should be defined somewhere else, not in the SQLLogicTestLogger::AddToSummary(string log_message) method, maybe globally, like the duckdb_unittest_tempdir name.

@duckdb-draftbot duckdb-draftbot marked this pull request as draft March 27, 2025 09:19
@hmeriann hmeriann marked this pull request as ready for review March 27, 2025 09:31
@duckdb-draftbot duckdb-draftbot marked this pull request as draft March 27, 2025 10:54
@hmeriann hmeriann marked this pull request as ready for review March 27, 2025 10:55
@hmeriann hmeriann requested a review from Tmonster March 27, 2025 10:55
@duckdb-draftbot duckdb-draftbot marked this pull request as draft March 27, 2025 11:23
@hmeriann hmeriann marked this pull request as ready for review March 27, 2025 11:23
@hmeriann
Copy link
Contributor Author

We still will have to scroll in case of long logs like this

@Tmonster
Copy link
Contributor

We still will have to scroll in case of long logs like this

I was going to say yea. I think if you summarize everything, it's a bit too long of a message. The reason we want this is just to see what files have failing test cases. It's much nicer to look at the files, then run the tests locally to find out exactly what has failed. If you want to have a summary with also the failures, then I suggest having two flags --summarize-failures and --summarize-failues-verbose, with verbose also publishing expected error messages etc.

@Mytherin
Copy link
Collaborator

I think the current approach is fine - showing the error again is helpful as well. This is useful if we want to e.g. check if an error is expected. Sometimes errors are also hard to reproduce locally.

The HTTPFS example is a bit excessive because there are many errors and the errors are in EXPLAIN statements - but that's not going to look great anyway. For smaller errors this looks great to me - e.g. https://github.com/duckdb/duckdb/actions/runs/14105375963/job/39511652190?pr=16833

Mytherin
Mytherin reviewed Mar 27, 2025
View reviewed changes
@hmeriann hmeriann marked this pull request as ready for review May 30, 2025 12:13
@hmeriann hmeriann requested a review from Tmonster May 30, 2025 14:00
@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 2, 2025 17:09
@hmeriann hmeriann marked this pull request as ready for review June 3, 2025 07:14
Mytherin
Mytherin reviewed Jun 11, 2025
View reviewed changes
Copy link
Collaborator
@Mytherin Mytherin left a comment

Choose a reason for hiding this comment

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

Can we pick this up again? After digging through some more logs this is still very much a needed change. I've left some more comments:

@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 11, 2025 17:17
@hmeriann hmeriann marked this pull request as ready for review June 12, 2025 15:17
@hmeriann hmeriann requested a review from Mytherin June 13, 2025 07:51
Mytherin
Mytherin reviewed Jun 13, 2025
View reviewed changes
Copy link
Collaborator
@Mytherin Mytherin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the fixes! Looks good. Some more comments below. I also noticed that we are not reporting time out failures in run_tests_one_by_one.py, see e.g. the "Release Assertions with Clang" run. Can we report those as well?

[2573/4670]: test/sql/join/asof/test_asof_join_missing.test_slowStill running...
Still running...
Still running...
(TIMED OUT)

@duckdb-draftbot duckdb-draftbot marked this pull request as draft June 13, 2025 11:34
@hmeriann hmeriann force-pushed the unittester-summary branch from 2d3d0f7 to 46df878 Compare June 15, 2025 07:56
@hmeriann hmeriann marked this pull request as ready for review June 15, 2025 07:57
@Mytherin Mytherin merged commit 48bcb44 into duckdb:main Jun 17, 2025
66 of 71 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mytherin Mytherin Mytherin left review comments

@Tishj Tishj Tishj left review comments

@carlopi carlopi Awaiting requested review from carlopi

@Tmonster Tmonster Awaiting requested review from Tmonster

Assignees
No one assigned
Labels
Changes Requested Ready For Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hmeriann @Mytherin @Tmonster @Tishj @carlopi
0