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.

Sign up for GitHub

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

Jump to bottom

Restore behaviour from 0.10-2 to .xts() #250

Merged
merged 2 commits into from
Jul 27, 2018
Merged

Restore behaviour from 0.10-2 to .xts() #250

merged 2 commits into from
Jul 27, 2018

Conversation

TomAndrews
Copy link
Contributor

@TomAndrews TomAndrews commented Jul 18, 2018 •
edited
Loading

Add unit tests to document the behaviour in 0.10-2 and fix the failures introduced in 0.11. Fixes #249.

@TomAndrews
Copy link
Contributor Author

Hmm the checks pass locally on R 3.5.1. I'll investigate.

@TomAndrews
Copy link
Contributor Author

I'm stumped, I can't replicate the test failures on any of the 3 machines I have easy access to. Any hints would be appreciated.

@joshuaulrich
Copy link
Owner

Since the failing test is for the tzone attribute, my guess is that the cause is an unset/different TZ environment variable. What is Sys.getenv("TZ") on your local machines? You might need to add a .setUp() or something to your tests to Sys.setenv(TZ = "UTC") or similar.

@TomAndrews
Copy link
Contributor Author

Yes, explicitly setting TZ in .setUp fixed it. Thanks!

@joshuaulrich joshuaulrich self-requested a review July 26, 2018 13:35
joshuaulrich
joshuaulrich reviewed Jul 26, 2018
View reviewed changes
Copy link
Owner

@joshuaulrich joshuaulrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, tests should cover the API, not the implementation. That said, it may make sense to keep the checks for the tclass and tzone attributes on the index, since they will be necessary once the index class and index timezone are attributes on the index, not the xts object.

I am interested in whether using the API's extractor functions would change the results of the test cases that pass in 0.10-2 but look wrong. It would be great if you could make those changes to the checkXts*() functions and re-test on 0.10-2.

inst/unitTests/runit.xts.R
@@ -50,3 +50,52 @@ test..xts_dimnames_in_dots <- function() {
y <- xts(1:5, index(x), dimnames = list(NULL, "x"))
checkEquals(x, y)
}

checkXtsClass <- function(xts, class) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test looks for attributes on specific objects, but it probably should use the extractor functions indexClass() and tclass().

inst/unitTests/runit.xts.R Outdated
checkXtsClass(.xts(1, structure(1, tzone="",tclass="yearmon"), tclass="timeDate", .indexCLASS="Date"), "Date")
}

checkXtsTz <- function(xts, tzone, .indexTZ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test looks for attributes on specific objects, but it probably should use the extractor functions indexTZ() and tzone().

@TomAndrews
Copy link
Contributor Author

I've added a commit to alter the two checkXts* functions so that they check the two accessor functions and the attribute on the index.

Testing in v0.10-2 (see my branch here) means that the 3 TZ checks are now always consistent - you don't ever see the differing values of .indexTZ and tzone.

It seems that for timezones in v0.10-2 specifying tzone on the xts object takes precedence over setting tzone on the index of the xts. .indexTZ always seems to be ignored, which is what is causing the last odd looking test case. I have updated the tests to reflect all this.

As it stands this pull request reinstates the behaviour from v0.10-2; the updated tests still pass with my original fixes. But you might want to do something different with the C code to set .indexTZ: it seems a bit pointless to pass it in and set it if it's never actually used.

Do let me know if you'd like any other changes.

@TomAndrews
Copy link
Contributor Author

In fact, on closer inspection, my changes to the handling of .indexTZ are not required to make the unit tests pass as long as you use the accessor functions and don't look at the attributes directly.

I can squash this down if you're happy.

@joshuaulrich
Copy link
Owner

But you might want to do something different with the C code to set .indexTZ: it seems a bit pointless to pass it in and set it if it's never actually used.

Thanks for noticing this and mentioning it. The plan is to deprecate .indexTZ and .indexClass (in favor of tzone and tclass on the index), so I'm not concerned that it's not currently being used.

This looks good to me now. I would appreciate a cleaned-up history (squash or rebase, depending on what you think tells the story better), especially:

  • adding to the commit message(s) any relevant parts of this conversation, and
  • See #249. and/or Fixes #249. on a line at the end of the commit message, so the commit appears in the issue tracker.

@TomAndrews
Add unit tests for .xts() checking index class and timezone
beb4267 
As documented in joshuaulrich#249, v0.11 introduced changes to .xts() which altered
behaviour compared to v0.10-2.  As a first step to fixing this
regression, this commit adds unit tests for all possible permutations
for setting the index class and timezone in .xts().

The tests check accessing the class in 3 different ways:
    1. Using tclass()
    2. Using indexClass()
    3. Checking the 'tclass' attribute on the index object

Similarly the tests check accessing timezone by:
    1. Using tzone()
    2. Using indexTZ()
    3. Checking the '.indexTZ' attribute on the index object

These tests are designed to document the behaviour as of v0.10-2.  As
such they pass on v0.10-2 but fail for v0.11.
@TomAndrews
Fix failure of test..xts_class
1d707c5 
Reinstate the behaviour as of v0.10-2.  .indexCLASS should take
precedence over tclass.

Fixes joshuaulrich#249.
@joshuaulrich joshuaulrich merged commit 1d707c5 into joshuaulrich:master Jul 27, 2018
@joshuaulrich
Copy link
Owner

Great work @TomAndrews! Thank you very much for your contributions!

@joshuaulrich joshuaulrich added this to the Release 0.11-1 milestone Jul 30, 2018
@TomAndrews TomAndrews deleted the constructor-unit-tests-11 branch January 21, 2020 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuaulrich joshuaulrich joshuaulrich left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Release 0.11-1
Development

Successfully merging this pull request may close these issues.

reclass() changes "tclass" attribute
2 participants
@TomAndrews @joshuaulrich