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

Fix timezone incosistencies between different ruby version #6697

Merged
merged 6 commits into from Jan 25, 2018

Conversation

Crunch09
Copy link
Member

@Crunch09 Crunch09 commented Jan 16, 2018

tl;dr SafeYAML with Ruby < 2.4 parses times in the timezone currently set in the environment while SafeYAML for Ruby 2.4 keeps times in the timezone in which they were dumped in.

As described in issues like #5963 and #6033 the current SafeYAML.load behaviour depends on the ruby version.

# ruby -v => ruby 2.3.3p222
ENV['TZ'] = 'Australia/Melbourne'
hash = {date: Time.now}       # => {:date=>2018-01-17 09:25:57 +1100}
SafeYAML.load YAML.dump(hash) # => {:date=>2018-01-17 09:25:57 +1100}
ENV['TZ'] = 'UTC'
SafeYAML.load YAML.dump(hash) # => {:date=>2018-01-16 22:25:57 +0000}
# ruby -v => ruby 2.4.2p198
ENV['TZ'] = 'Australia/Melbourne'
hash = {date: Time.now}       # => {:date=>2018-01-17 09:25:57 +1100}
SafeYAML.load YAML.dump(hash) # => {:date=>2018-01-17 09:25:57 +1100}
ENV['TZ'] = 'UTC'
SafeYAML.load YAML.dump(hash) # => {:date=>2018-01-17 09:25:57 +1100}

This scenario should have caught this, but unfortunately the dates in our scenarios were read as String instead of Time (which doesn't happen with a real frontmatter, see unit tests) so the issue didn't occur.
5365355 should fix that and make this scenario fail in Ruby 2.4

Scenario: Generate proper dates with explicitly set timezone (different than posts' time)
Given I have a _layouts directory
And I have a page layout that contains "Page Layout: {{ site.posts.size }}"
And I have a post layout that contains "Post Layout: {{ content }} built at {{ page.date | date_to_xmlschema }}"
And I have an "index.html" page with layout "page" that contains "site index page"
And I have a configuration file with:
| key | value |
| timezone | Pacific/Honolulu |
And I have a _posts directory
And I have the following posts:
| title | date | layout | content |
| entry1 | 2013-04-09 23:22 +0400 | post | content for entry1. |
| entry2 | 2013-04-10 03:14 +0400 | post | content for entry2. |
When I run jekyll build
Then I should get a zero exit status
And the _site directory should exist
And I should see "Page Layout: 2" in "_site/index.html"
And the "_site/2013/04/09/entry1.html" file should exist
And the "_site/2013/04/09/entry2.html" file should exist
And I should see "Post Layout: <p>content for entry1.</p>\n built at 2013-04-09T09:22:00-10:00" in "_site/2013/04/09/entry1.html"
And I should see "Post Layout: <p>content for entry2.</p>\n built at 2013-04-09T13:14:00-10:00" in "_site/2013/04/09/entry2.html"

I couldn't test so far locally with Ruby 2.5 but think the behaviour should stay the same as it is with Ruby 2.4

cc: @parkr Made a PR out of this because it might be easier to discuss and also CI runs then.

Crunch09 added 3 commits Jan 11, 2018
it fails with ruby 2.4 onwards and passes up to ruby 2.3
In a real application this would have already been read as a date so
make sure our scenario replicates that behaviour.
@Crunch09 Crunch09 changed the title WIP: Failing tests for timezone issues Failing tests for timezone issues Jan 17, 2018
Crunch09 added 2 commits Jan 17, 2018
This makes sure we don't have inconsistencies between different ruby
versions.
@@ -426,7 +426,7 @@ def merge_categories!(other)

private
def merge_date!(source)
if data.key?("date") && !data["date"].is_a?(Time)
if data.key?("date")
Copy link
Member

@pathawks pathawks Jan 18, 2018

Choose a reason for hiding this comment

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

I see the addition of tests 👍

What’s this line about?

Copy link
Member Author

@Crunch09 Crunch09 Jan 18, 2018

Choose a reason for hiding this comment

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

Sorry, didn't had time yesterday to properly update this PR with more information.

This change actually fixes the inconsistent behaviour, because as long as data['date'] is set it will call Utils.parse_date which transforms the given data into localtime (as determined by ENV['TZ']). This is exactly the behaviour that SafeYAML was doing automatically before Ruby 2.4.
Because of the old version of this line, there were no inconsistencies between the different Ruby versions when data['date'] was interpreted as a String rather than a Time.

That said, I am not sure the behaviour of Ruby 2.3 is my preferred one, i would actually prefer the behaviour of newer ruby versions but the most important thing is to fix the incosistency and i would argue that jekyll users are more used to the behaviour of everything gets converted into whatever ENV['TZ'] is.

@Crunch09 Crunch09 changed the title Failing tests for timezone issues Fix timezone incosistencies between different ruby version Jan 18, 2018
@Crunch09
Copy link
Member Author

Crunch09 commented Jan 18, 2018

@stomar @yous @Manishearth @t-brink Would love to hear your thoughts on this as you were quite involved in past discussions of this issue.

cc: @jekyll/build

@DirtyF DirtyF requested a review from a team Jan 18, 2018
@Crunch09
Copy link
Member Author

Crunch09 commented Jan 18, 2018

@parkr It's not visible in the changed files so i mention it now here: This scenario

Scenario: Generate proper dates with explicitly set timezone (different than posts' time)
would now fail in newer Ruby versions without the fix. (Travis log of failure)

@yous
Copy link
Contributor

yous commented Jan 19, 2018

In my local machine with KST timezone,

$ ruby -e 'puts ENV["TZ"].inspect'
nil
$ date +"%Z %z"
KST +0900

The test features/permalinks.feature:73 fails. I added TEST step to show the file entries in _site.

$ bundle exec cucumber features/permalinks.feature:73

features/permalinks.feature:73  Scenario: Use custom permalink schema with date and time Feature: Fancy permalinks
  As a hacker who likes to blog
  I want to be able to set permalinks
  In order to make my blog URLs awesome

  Scenario: Use custom permalink schema with date and time            # features/permalinks.feature:73
    Given I have a _posts directory                                   # features/step_definitions.rb:68
    And I have the following post:                                    # features/step_definitions.rb:76
      | title                   | category | date                | content         |
      | Custom Permalink Schema | stuff    | 2009-03-27 22:31:07 | Totally custom. |
    And I have a configuration file with:                             # features/step_definitions.rb:125
      | key       | value                                      |
      | permalink | "/:year:month:day:hour:minute:second.html" |
      | timezone  | UTC                                        |
    When I run jekyll build                                           # features/step_definitions.rb:157
    Then I should get a zero exit status                              # features/step_definitions.rb:361
    And the _site directory should exist                              # features/step_definitions.rb:215
    And TEST                                                          # features/step_definitions.rb:289
      [".", "..", "20090327133107.html"]
    And I should see "Totally custom." in "_site/20090327223107.html" # features/step_definitions.rb:225
      expected #<Pathname:_site/20090327223107.html> to exist (RSpec::Expectations::ExpectationNotMetError)
      ~/src/jekyll/features/step_definitions.rb:310:in `/^the "(.*)" file should +(not )?exist$/'
      ~/src/jekyll/features/step_definitions.rb:226:in `/^I should (not )?see "(.*)" in "(.*)"$/'
      features/permalinks.feature:86:in `And I should see "Totally custom." in "_site/20090327223107.html"'
 (0.580991s)

Worst offenders:
  0.580991s for "Use custom permalink schema with date and time" (features/permalinks.feature:73)

Failing Scenarios:
cucumber features/permalinks.feature:73 # Scenario: Use custom permalink schema with date and time

1 scenario (1 failed)
8 steps (1 failed, 7 passed)
0m0.581s

Is this intentional? I didn't set ENV['TZ'] explicitly and Jekyll's configuration contains timezone: UTC, but the permalink is created based on KST.

@Crunch09
Copy link
Member Author

Crunch09 commented Jan 19, 2018

@yous Thanks! Looking at that test.

In a real world example the post frontmatter would only be read during
the build step after the configuration has already been applied. In a
cucumber scenario though the frontmatter is read outside of any
configuration so in this case the `timezone` setting of `UTC` wasn't applied.
@Crunch09
Copy link
Member Author

Crunch09 commented Jan 20, 2018

@yous This issue only occured in a test and wouldn't have happened in a real application. This is because unlike during a real build, the frontmatter of the post is read before any configuration has been applied (in this case timezone). Even though your ENV['TZ'] is not set Ruby still recognizes the time / timezone of your computer, you can test this with:

Time.new # 2018-01-20 20:58:12 +0900

e3b32f3 now makes sure that the timezone of the scenario is set before reading any post frontmatter.
Many thanks again for catching this 👍 I couldn't see this behaviour earlier because i'm living in the UTC timezone and i assume CI tests also run in UTC. Please let me know if this scenario (or any other one) is still failing for you.

@yous
Copy link
Contributor

yous commented Jan 20, 2018

@Crunch09 Now every test passes. Thanks for the quick fix and explanation. 👍

@DirtyF DirtyF added this to the v3.8.0 milestone Jan 20, 2018
ghost
ghost approved these changes Jan 23, 2018
Copy link

@ghost ghost left a comment

I'd love to have someone from @jekyll/build review this before we land it!

@ghost
Copy link

ghost commented Jan 23, 2018

Additionally, we can probably squeeze this in v3.7.1. What do you think, @jekyll/core?

@DirtyF DirtyF modified the milestones: v3.8.0, v3.7.1 Jan 23, 2018
@ashmaroli ashmaroli mentioned this pull request Jan 23, 2018
7 tasks
@ghost
Copy link

ghost commented Jan 25, 2018

Merging this, assuming nobody has any further qualms with it.

@ghost
Copy link

ghost commented Jan 25, 2018

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit cf5c689 into master Jan 25, 2018
@jekyllbot jekyllbot deleted the timezone-issues branch Jan 25, 2018
jekyllbot added a commit that referenced this pull request Jan 25, 2018
@Crunch09
Copy link
Member Author

Crunch09 commented Jan 25, 2018

Thanks @pup ! 🙇

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants