ATFutures / calendar

R interface to iCal (.ics files)
https://atfutures.github.io/calendar/
Other
41 stars 11 forks source link

Failing incoming CRAN checks #50

Closed Robinlovelace closed 1 month ago

Robinlovelace commented 5 months ago

I spoke too soon in #49, it bounced from CRAN, with the following message:

package calendar_0.1.0.tar.gz does not pass the incoming checks automatically, please see the following pre-tests: Windows: https://win-builder.r-project.org/incoming_pretest/calendar_0.1.0_20240427_153111/Windows/00check.log Status: 1 ERROR Debian: https://win-builder.r-project.org/incoming_pretest/calendar_0.1.0_20240427_153111/Debian/00check.log Status: 1 ERROR

Last released version's CRAN status: OK: 12 See: https://cran.r-project.org/web/checks/check_results_calendar.html

CRAN Web: https://cran.r-project.org/package=calendar

Please fix all problems and resubmit a fixed version via the webform. If you are not sure how to fix the problems shown, please ask for help on the R-package-devel mailing list: https://stat.ethz.ch/mailman/listinfo/r-package-devel If you are fairly certain the rejection is a false positive, please reply-all to this message and explain.

More details are given in the directory: https://win-builder.r-project.org/incoming_pretest/calendar_0.1.0_20240427_153111/ The files will be removed after roughly 7 days.

Strong rev. depends : stRoke

Best regards, CRAN teams' auto-check service Flavor: r-devel-linux-x86_64-debian-gcc, r-devel-windows-x86_64 Check: CRAN incoming feasibility, Result: Note_to_CRAN_maintainers Maintainer: 'Robin Lovelace [rob00x@gmail.com](mailto:rob00x@gmail.com)'

Flavor: r-devel-windows-x86_64 Check: tests, Result: ERROR Running 'testthat.R' Running the tests in 'tests/testthat.R' failed. Complete output:

library(testthat) library(calendar)

test_check("calendar") [ FAIL 1 | WARN 1 | SKIP 0 | PASS 11 ]

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-ic-extract.R:4:3'): ic_extract works ─────────────────────────
ic_extract(ical_example, "DTSTART") not equal to as.POSIXct("2018-08-09 16:00:00 BST").
1/1 mismatches
[1] 2018-08-09 18:00:00 - 2018-08-09 16:00:00 == 2 hours

[ FAIL 1 | WARN 1 | SKIP 0 | PASS 11 ]
Error: Test failures
Execution halted

Flavor: r-devel-linux-x86_64-debian-gcc Check: tests, Result: ERROR Running 'testthat.R' [1s/1s] Running the tests in 'tests/testthat.R' failed. Complete output:

library(testthat) library(calendar)

test_check("calendar") [ FAIL 1 | WARN 1 | SKIP 0 | PASS 11 ]

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure ('test-ic-extract.R:4:3'): ic_extract works ─────────────────────────
ic_extract(ical_example, "DTSTART") not equal to as.POSIXct("2018-08-09 16:00:00 BST").
1/1 mismatches
[1] 2018-08-09 18:00:00 - 2018-08-09 16:00:00 == 2 hours

[ FAIL 1 | WARN 1 | SKIP 0 | PASS 11 ]
Error: Test failures
Execution halted

Anyone know how to fix this? I'm a bit bamboozled as the tests pass locally and on actions as far as I can tell? Any support very, very welcome :pray:

silverfoxdoc commented 5 months ago

I'm thinking that's because of my pull request. Should have done the testthat scripts before submitting pull request but not got any real experience using testthat.

DTSTART:20180809T160000Z in ical_example is in zulu time; therefore given August is BST time zone and we're converting the zulu time zone to local it would actually be UTC+1 and so 17:00

So I think we should be testing against as.POSIXct("2018-08-09 17:00:00 BST") rather than 16:00 now that we're converting zulu times to local times

Robinlovelace commented 5 months ago

Yes makes sense @silverfoxdoc.. Quick fix? Long time since I looked at the code as you can tell!

Robinlovelace commented 5 months ago

Fix could be here:

https://github.com/ATFutures/calendar/blob/c4ca30a7822cd312fd53f8067648bfa44c7dd8fb/tests/testthat/test-ic-extract.R#L3-L6

Robinlovelace commented 5 months ago

Or (I think less likely) here: https://github.com/ATFutures/calendar/blob/master/inst/extdata/example.ics

serkor1 commented 1 month ago

Is this still a standing issue?

Robinlovelace commented 1 month ago

Yes as far as I know.

mpadge commented 1 month ago

This kind of issue happens all the time in GTFS-world, which is timestamps all the way down. All tests simply have to ensure that they hard-code the same zone throughout. Any test which doesn't will run in the local tz of whatever machine it is run on, and thus likely fail. It doesn't really matter what tz is used, as long as they are consistent. lubrdate has useful force_tz() and with_tz() functions to help.

serkor1 commented 1 month ago

This kind of issue happens all the time in GTFS-world, which is timestamps all the way down. All tests simply have to ensure that they hard-code the same zone throughout. Any test which doesn't will run in the local tz of whatever machine it is run on, and thus likely fail. It doesn't really matter what tz is used, as long as they are consistent. lubrdate has useful force_tz() and with_tz() functions to help.

I agree on this. The error-messages suggests that its time zone that is the sinner here. The time zones are determined where ever the test environment is located - for Github its US, and CRAN is Germany or Austria (Correct me if I am wrong).

Robinlovelace commented 1 month ago

Does anyone have examples of the test?

I guess wrapping with tests in with_tz() would do it. Great tip that could save time if so, big if true!

serkor1 commented 1 month ago

@Robinlovelace the closest example I have is the tests I have in {cryptoQuotes}.

In the linked test file I, instead of testing for equality, test wether the parsed dates are sensible. Could be that such a strategy is more feasible - not saying it's better than using {lubridate}, but its feasible.

Robinlovelace commented 1 month ago

Let's see around Monday, CRAN team back from their hard-earned holidays then!

serkor1 commented 1 month ago

Let's see around Monday, CRAN team back from their hard-earned holidays then!

Fingers crossed!

serkor1 commented 1 month ago

Let's see around Monday, CRAN team back from their hard-earned holidays then!

How did it go?

silverfoxdoc commented 1 month ago

I can't quite remember if I fixed this in my other pull request which was trying to add functionality. If I get time this coming weekend I will try to have a look at it again. I was trying avoid using lubridate as this adds another dependancy, although it is very convenient to use.

Robinlovelace commented 1 month ago

Thanks for the reminder, I knew there was something (else) I needed to do today!

Robinlovelace commented 1 month ago

This is fixed.