Skip to content

Conversation

niykko
Copy link
Contributor

@niykko niykko commented May 26, 2025

Currently, no information about the hashes is shown when a unittest fails because of a hash value mismatch, even though this information is readily available.

This small PR aims to include this information in the output while also making it less confusing: before, it printed an 'Expected result' header without supplying any results (or rather, printing it in the previous lines before the introductory header).

	PrintHeader("Expected result:");
	PrintLineSep();        <--- There should be a result here
	PrintHeader("Actual result:");

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.

Thanks! Can you just have a look at the failing CI?

@@ -46,7 +46,7 @@ class SQLLogicTestLogger {
void ColumnCountMismatchCorrectResult(idx_t original_expected_columns, idx_t expected_column_count,
MaterializedQueryResult &result);
void SplitMismatch(idx_t row_number, idx_t expected_column_count, idx_t split_count);
void WrongResultHash(QueryResult *expected_result, MaterializedQueryResult &result);
void WrongResultHash(QueryResult *expected_result, MaterializedQueryResult &result, string &expected_hash, string &actual_hash);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can these be const references instead?

@duckdb-draftbot duckdb-draftbot marked this pull request as draft May 27, 2025 13:12
@niykko niykko marked this pull request as ready for review May 27, 2025 13:13
@Mytherin Mytherin merged commit f87449e into duckdb:main May 28, 2025
40 checks passed
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request Jun 5, 2025
New Sorting Implementation (duckdb/duckdb#17584)
Output hashes in unittest and fix order (duckdb/duckdb#17664)
@niykko niykko deleted the testfix branch August 4, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants