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

Update R tests to run test_struct.R not on CRAN #8297

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

Tmonster
Copy link
Contributor

There have been some issues with R tests lately. It turns out that importing the arrow library after running R relational tests can cause the R garbage collector to throw some errors. We want to avoid these errors at all costs on CRAN, but we want to run the arrow tests in our own CI. This PR re-enables running all tests in test_struct.R. In addition, it removes a python file in the R tests that isn't needed. It also adds a helper function to import the arrow library first so that this issue doesn't happen again.

@Tmonster Tmonster requested review from carlopi and Tishj July 18, 2023 14:16
Copy link
Contributor

@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 for reverting the hack and putting this into proper shape. Good for me!

I am not sure what role the test_arrow_recorbatchreader.py had, pinging @pdet that according to git log put it there to check if has anything against the removal.

@pdet
Copy link
Contributor

pdet commented Jul 18, 2023

LGTM!
The file is a python arrow test, it was probably miscopied to the R folder. It already exists in the tools/pythonpkg/tests/fast/arrow folder.

@Mytherin Mytherin merged commit b8fe100 into duckdb:master Jul 18, 2023
7 checks passed
@Mytherin
Copy link
Collaborator

Thanks!

krlmlr pushed a commit to krlmlr/duckdb that referenced this pull request Aug 21, 2023
Update R tests to run test_struct.R not on CRAN
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.

None yet

4 participants