Skip to content
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.

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

fix unit tests #1547

Merged
merged 5 commits into from
Jan 2, 2024
Merged

fix unit tests #1547

merged 5 commits into from
Jan 2, 2024

Conversation

jraymakers
Copy link
Contributor

A few unit tests were consistently failing for me locally. This should fix them.

The all_types test was missing the "union" type, which was recently added to test_all_types().
(The other changes to this file are automatic formatting fixes applied by VS Code according to the settings in the repo.)

A few tests relied on generated TPCH files. The generation script was failing because it didn't load the (newly loadable) tpch extension. I added the appropriate "install" and "load" commands to this script.

@jraymakers
Copy link
Contributor Author

Strangely, without this change, the all_types test fails in my local environment with Expected 44 to equal 43., but with this change, the GitHub CI build fails with Expected 43 to equal 44. I don't understand why the expected numbers are different.

@jraymakers
Copy link
Contributor Author

Oh, it looks like the CI applies a patch to duckdb (duckdb.patch), and part of that patch is to remove the union column from test_all_types(). This patch is not applied in a local build.

@jraymakers
Copy link
Contributor Author

I've removed the patch that removed union from test_all_types in the CI. This caused the C++ all_types_test to fail, since it didn't have a test for union, so I added that.

Since the published version has union, this should bring the CI version closer to the published version.

Copy link
Collaborator

@carlopi carlopi left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR, makes a lot of sense to me.

@Mytherin Mytherin merged commit fd4e276 into duckdb:main Jan 2, 2024
15 checks passed
@Mytherin
Copy link
Contributor

Mytherin commented Jan 2, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants