Make tests 168,2043 work for any LC_TIME

[tdhock]

Closes #3450
hi @jangorecki @shrektan @2005m @MichaelChirico this is a fix for #3450 which you commented on.
The problem in that issue is that some of the computed values depend on the LC_TIME locale value, whereas the expected values in the test are always English/C locale (Jan/NOV).
On my system (ubuntu 18.04) I have the following locales, the important one is LC_TIME=fr_FR.UTF-8

Fri Sep 25 16:41:56 2020  endian==little, sizeof(long double)==16, longdouble.digits==64, sizeof(pointer)==8, TZ==unset, Sys.timezone()=='America/Phoenix', Sys.getlocale()=='LC_CTYPE=en_US.UTF-8;LC_NUMERIC=C;LC_TIME=fr_FR.UTF-8;LC_COLLATE=en_US.UTF-8;LC_MONETARY=fr_FR.UTF-8;LC_MESSAGES=en_US.UTF-8;LC_PAPER=fr_FR.UTF-8;LC_NAME=C;LC_ADDRESS=C;LC_TELEPHONE=C;LC_MEASUREMENT=fr_FR.UTF-8;LC_IDENTIFICATION=C', l10n_info()=='MBCS=TRUE; UTF-8=TRUE; Latin-1=FALSE', getDTthreads()=='omp_get_num_procs()==2; R_DATATABLE_NUM_PROCS_PERCENT==unset (default 50); R_DATATABLE_NUM_THREADS==unset; R_DATATABLE_THROTTLE==unset (default 1024); omp_get_thread_limit()==2147483647; omp_get_max_threads()==2; OMP_THREAD_LIMIT==unset; OMP_NUM_THREADS==unset; RestoreAfterFork==true; data.table is using 1 threads with throttle==1024. See ?setDTthreads.'

with current master I get

(base) tdhock@maude-MacBookPro:~/R$ R CMD INSTALL data.table && R --vanilla < data.table/tests/main.R 
...
  7 errors out of 9996. Search tests/tests.Rraw.bz2 for test numbers: 168.1, 168.2, 168.3, 2042.2, 2042.3, 2127.26, 2127.27.

After applying the commit in this PR I get

All 10001 tests in tests/tests.Rraw completed ok in 00:01:50 elapsed (00:01:46 cpu)

I could have fixed this by just setting LC_TIME=C during the tests, but I figured it would be better to not alter the environment. So instead I just changed the expected values so that they are generated based on the current locale (e.g. "janv." and "nov." instead of "Jan" and "NOV"). Is this approach ok?

[codecov]

Codecov Report

Merging #4719 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4719   +/-   ##
=======================================
  Coverage   99.44%   99.44%           
=======================================
  Files          73       73           
  Lines       14539    14539           
=======================================
  Hits        14458    14458           
  Misses         81       81           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cc5c0c...e3bc071. Read the comment docs.

[MichaelChirico]

LGTM thanks!

[tdhock]

great thanks! @mattdowle would you consider merging please?

[MichaelChirico]

hi Toby, any particular reason to prioritize? As you can see we've got ourselves a bit of a PR backlog at the moment

…

On Sat, Sep 26, 2020, 2:15 AM Toby Dylan Hocking ***@***.***> wrote: great thanks! @mattdowle <https://github.com/mattdowle> would you consider merging please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4719 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2BA5NGCENXIPHTTIWVFGTSHWBJHANCNFSM4R2QRIYA> .

[tdhock]

haha no rush

[mattdowle]

Great. Had a look. It just needs a short news item (notes section), and the ", #3450" adding on these lines please.

[mattdowle]

As it happens, since CRAN is in error status and this release is pressing, the timing is good at the moment for any quick and easy PRs (like this one) to be prioritized to get them out of the way in 1.13.2. Most of the PRs currently scheduled for next release (milestone 1.13.1) will probably get bumped to 1.13.3 milestone.

[tdhock]

hi @mattdowle thanks for the quick review. here are the issue/PR mentions, is that OK?

[mattdowle]

Thanks @tdhock. News items have developed a silent standard we should probably write down. We just reference the issue and not the PR too, because the issue itself links to the PR. That keeps the news item briefer, but also 1 link rather than 2 helps the CRAN check times because CRAN checks that every link in NEWS.md works and that can take over 20 mins. Also the name of the user-visible function affected should be present, in this case test.data.table(), and up front, to aid users quickly scanning the news items. It's ok I can tweak the news item, just explaining what's in my mind when I tweak them. We should write this stuff down.

[mattdowle]

@tdhock is the revised news item ok? I'm not sure about the "Jan vs janv." part. Is it J vs j like that, and the French has a dot too? Yep, I set LC_TIME myself and confirmed it is.

[tdhock]

great thanks for the info