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

Tests robust to locale sorting #6074

Merged
merged 5 commits into from
Apr 19, 2024
Merged

Tests robust to locale sorting #6074

merged 5 commits into from
Apr 19, 2024

Conversation

MichaelChirico
Copy link
Member

Closes #3502.

@minemR it would be great if you could confirm this works on Windows as well, but I don't have any reason to believe there's an issue there.

@tdhock
Copy link
Member

tdhock commented Apr 19, 2024

I did a PR to fix some tests on a machine where I run a French locale, #4719
The approach I used there was not setting the locale, and instead making the expected values depend on the locale too.
This approach in this PR is setting the locale to C for the duration of the test, then setting back to original locale.
I don't have any problem with this approach, just wanted to mention the difference.

@MichaelChirico
Copy link
Member Author

I mentioned an approach like that in the linked issue: #3502 (comment)

I might be missing something simpler, but the approach I was seeing to make the test reference depend on the locale was pretty abstruse, hence going with temporarily editing the locale.

IIUC, in #4719, data.table and the reference value both depend on the underlying locale setting, which may be why it's easier to avoid temporary settings in that case. Here, data.table's value is completely independent of the setting.

Actually, this discussion is reminding me that, because base R uses data.table's own code for method="radix", it's also independent of locale, so possibly that will work instead (though not for factor leveling, easily). However, that's only available in R>=3.3.0. I'll add a TODO to revisit that once we have a high enough R dependency.

@MichaelChirico MichaelChirico merged commit b9d51f1 into master Apr 19, 2024
3 checks passed
@MichaelChirico MichaelChirico deleted the test-with-c-collate branch April 19, 2024 16:32
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.

test.data.table report forderv
2 participants