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

Workaround failing test on Alpine Linux #6366

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Conversation

MichaelChirico
Copy link
Member

@bastistician
Copy link

I'd suggest to fix this for these C-locale tests just like in the tests 1590.12/14 below:
if x1==x2 (as seen on Alpine Linux), then test that forderv(c(x2,x1,x1,x2)) gives integer(), otherwise INT(1,4,2,3).

@tdhock
Copy link
Member

tdhock commented Aug 14, 2024

I'm not sure I really understand the core issue this fixes, but I guess it is OK to skip a couple of tests on one platform, temporarily. I would suggest keeping the issue open and hopefully @bastistician can help suggest a longer term fix/PR?

@MichaelChirico
Copy link
Member Author

it really takes some heavy focus time for me to wrap my head around encoding issues, so I'd rather kick the can here/not hold up release waiting for a fix. leaving the issue open to revisit for 1.17.0

@MichaelChirico MichaelChirico merged commit 551c788 into master Aug 14, 2024
3 checks passed
@MichaelChirico MichaelChirico deleted the skip-encoding-tests branch August 14, 2024 14:21
@bastistician
Copy link

hopefully @bastistician can help suggest a longer term fix/PR?

Sure, see my suggestion above, i.e., test the forderv() result depending on x1==x2.
Conditioning on Alpine Linux specifically will make my check system happy but won't help other musl-based distros (where I'd expect the same test failures).

@MichaelChirico
Copy link
Member Author

I agree it's better, but it's not a priority at the moment. That code has been in data.table since 1.11.4 (2018: 2b4d73613), with yours the first user report.

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