Update tests for the timezone-data package #24562
Conversation
WalkthroughThe timezone fixture array at ghost/admin/mirage/fixtures/timezones.js was edited: several timezone entries were added, removed or renamed (notably replacing America/Tijuana with America/Mazatlan, adding America/Chihuahua, swapping/adjusting various Asia entries and labels), and GMT offsets/labels were corrected for multiple regions. The integration test ghost/admin/tests/integration/services/config-test.js had its expected availableTimezones length updated to reflect the changed fixture data (assertion updated from 67 to 70). Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related issues
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/admin/mirage/fixtures/timezones.js (3)
17-17: Remove trailing whitespace.There's unnecessary trailing whitespace after the comma.
- }, + },
203-204: Consider updating the city name spelling.The timezone entry is correct, but consider updating "Katmandu" to "Kathmandu" for the more modern and widely accepted spelling.
- label: '(GMT +5:45) Katmandu' + label: '(GMT +5:45) Kathmandu'
221-221: Fix formatting: add space after comma.The addition of "Novosibirsk" is appropriate, but there should be a space after the comma for consistency.
- label: '(GMT +7:00) Krasnoyarsk,Novosibirsk' + label: '(GMT +7:00) Krasnoyarsk, Novosibirsk'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/mirage/fixtures/timezones.js(6 hunks)ghost/admin/tests/integration/services/config-test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
🔇 Additional comments (12)
ghost/admin/mirage/fixtures/timezones.js (11)
15-16: LGTM! Valid timezone identifier replacement.The replacement of
America/TijuanawithAmerica/Mazatlanis a legitimate timezone data update that aligns with standard IANA timezone database practices.
30-33: LGTM! Valid new timezone entry.The addition of
America/Chihuahuais a legitimate timezone entry with correct GMT offset and appropriate labeling.
64-64: LGTM! Enhanced timezone label for better geographic coverage.The addition of "Georgetown" to the America/Caracas label accurately reflects the cities covered by this timezone.
68-68: LGTM! Corrected timezone label by removing geographically incorrect city.Removing "Brasilia" from the America/Halifax label is correct, as Brasilia operates on GMT-3, not GMT-4 like Atlantic Canada.
80-80: LGTM! Corrected geographic assignment in timezone label.Changing "Georgetown" to "Brasilia" in the America/Argentina/Buenos_Aires label is accurate, as Brasilia operates on GMT-3 like Buenos Aires.
182-185: LGTM! Valid new timezone entry for Kazakhstan.The addition of
Asia/Almatywith GMT+5:00 offset and appropriate city labels is a legitimate timezone entry.
196-196: LGTM! Updated city name to current official designation.Changing "Calcutta" to "Kolkata" reflects the official name change and is more culturally appropriate.
199-201: LGTM! Accurate timezone entry for Sri Lanka.The Asia/Colombo entry with both major Sri Lankan cities in the label is geographically correct.
209-209: LGTM! Simplified timezone label.The simplified "Dhaka" label is appropriate for the Asia/Dhaka timezone.
213-213: LGTM! Inclusive labeling with both historical and current city names.Using "Yangon (Rangoon)" provides clarity by showing both the current official name and the historically known name.
229-229: LGTM! Corrected spelling of Mongolia's capital.The updated spelling "Ulaanbaatar" is the correct modern transliteration of Mongolia's capital city.
ghost/admin/tests/integration/services/config-test.js (1)
25-25: LGTM! Test updated to reflect increased timezone count.The change from 67 to 69 timezones correctly reflects the addition of "America/Chihuahua" and "Asia/Almaty" entries in the fixtures.
niranjan-uma-shankar
changed the title
niranjan-uma-shankar
changed the title
Updates the timezone data array size count to 70, given that the SDK package was updated with an additional entry
niranjan-uma-shankar
force-pushed
the
fix-tests-outdated-timezone
branch
from
ca71b3c to
a971529
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
ghost/admin/mirage/fixtures/timezones.js (2)
15-17: Verify the placement order of America/MazatlanThe America/Mazatlan entry (GMT -7:00) appears before America/Los_Angeles (GMT -8:00), which breaks the ascending GMT offset order. Consider moving it after line 21 to maintain consistent ordering.
Apply this diff to maintain consistent GMT offset ordering:
- { - name: 'America/Mazatlan', - label: '(GMT -7:00) La Paz, Mazatlan' - }, { name: 'America/Los_Angeles', label: '(GMT -8:00) Pacific Time (US & Canada); Tijuana' }, + { + name: 'America/Mazatlan', + label: '(GMT -7:00) La Paz, Mazatlan' + },
221-221: Add missing space after comma in labelThe label "Krasnoyarsk,Novosibirsk" is missing a space after the comma for consistency with other multi-city labels.
Apply this diff to fix the formatting:
- label: '(GMT +7:00) Krasnoyarsk,Novosibirsk' + label: '(GMT +7:00) Krasnoyarsk, Novosibirsk'ghost/admin/tests/integration/services/config-test.js (1)
25-30: Consider adding test coverage for the modified timezone entriesWhile the total count verification is correct, consider adding assertions to verify some of the newly added or modified timezone entries (e.g., America/Mazatlan, America/Chihuahua, Asia/Almaty) to ensure the fixture changes are fully validated.
Example additional assertions to add after line 30:
// Verify some of the newly added/modified entries const mazatlan = timezones.find(tz => tz.name === 'America/Mazatlan'); expect(mazatlan).to.exist; expect(mazatlan.label).to.equal('(GMT -7:00) La Paz, Mazatlan'); const chihuahua = timezones.find(tz => tz.name === 'America/Chihuahua'); expect(chihuahua).to.exist; expect(chihuahua.label).to.equal('(GMT -6:00) Chihuahua');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/mirage/fixtures/timezones.js(6 hunks)ghost/admin/tests/integration/services/config-test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
🔇 Additional comments (2)
ghost/admin/mirage/fixtures/timezones.js (1)
182-205: Offsets Verified – No Changes RequiredAll reviewed entries match current UTC offsets:
- Asia/Almaty – UTC +05:00
- Asia/Colombo – UTC +05:30
- Asia/Kathmandu – UTC +05:45
No update to these fixtures is needed.
ghost/admin/tests/integration/services/config-test.js (1)
25-25: Test correctly updated to match fixture dataThe test expectation has been properly updated from 67 to 70 timezones, matching the changes in the timezone fixtures file (removed 1, added 4 entries).
This PR updates the timezone data test and fixtures for the timezone data, which is needed for the changes made to the
timezone-datapackage in TryGhost/SDK#585.Ref: #24536
Note:
TryGhost/SDK#585 will need to be shipped for the tests here to pass. In the meantime, run the following test locally to verify they pass: