Skip to content

test: fix test-datetime-change-notify after daylight change #40670

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

test: fix test-datetime-change-notify after daylight change #40670

wants to merge 1 commit into from

Conversation

PiotrRybak
Copy link
Contributor

Add standard timezone name for Dublin without daylight saving

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Oct 31, 2021
@PiotrRybak
Copy link
Contributor Author

test-linux has failed in #40669 on test-datetime-change-notfy because of today's change in timezone related to daylight saving

@targos targos added the fast-track PRs that do not need to wait for 48 hours to land. label Oct 31, 2021
@github-actions
Copy link
Contributor

Fast-track has been requested by @targos. Please 👍 to approve.

@targos
Copy link
Member

targos commented Oct 31, 2021

@PiotrRybak thanks for the PR! Could you please update the commit message to fix the typo (notfy -> notify) ?

@PiotrRybak
test: fix test-datetime-change-notify after daylight change
db6785e 
Add standard timezone name for Dublin without daylight saving
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40591/

@PiotrRybak PiotrRybak changed the title test: fix test-datetime-change-notfy after daylight change test: fix test-datetime-change-notify after daylight change Oct 31, 2021
@PiotrRybak
Copy link
Contributor Author

@PiotrRybak thanks for the PR! Could you please update the commit message to fix the typo (notfy -> notify) ?

Fixed it, sorry about that

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40594/

tniessen
tniessen approved these changes Oct 31, 2021
View reviewed changes
test/parallel/test-datetime-change-notify.js
@@ -26,7 +26,7 @@ const cases = [
},
{
timeZone: 'Europe/Dublin',
expected: /Irish/,
expected: /(Irish Standard Time|Greenwich Mean Time)/,
Copy link
Member

Choose a reason for hiding this comment

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

It's probably best to avoid unnecessary groups:

Suggested change
expected: /(Irish Standard Time|Greenwich Mean Time)/,
expected: /Irish Standard Time|Greenwich Mean Time/,

@nodejs-github-bot

This comment has been minimized.

# to view
@nodejs-github-bot

This comment has been minimized.

# to view
@Mesteery Mesteery added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 31, 2021
@github-actions github-actions bot mentioned this pull request Nov 1, 2021
Open
38 tasks
@nodejs-github-bot

This comment has been minimized.

# to view
@nodejs-github-bot

This comment has been minimized.

# to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40605/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40606/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40607/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40608/

@Trott Trott mentioned this pull request Nov 1, 2021
Merged
@targos targos mentioned this pull request Nov 1, 2021
Closed
@tniessen
Copy link
Member

tniessen commented Nov 1, 2021

Note that #40684 exists now.

@PiotrRybak PiotrRybak closed this Nov 1, 2021
@Trott
Copy link
Member

Trott commented Nov 1, 2021

Note that #40684 exists now.

It retains @PiotrRybak as the author of the commit that fixes this. Thanks, @PiotrRybak!

@tniessen
Copy link
Member

tniessen commented Nov 1, 2021

For the record, this PR was effectively merged as 747ef34. Thank you for the contribution @PiotrRybak!

This was referenced Nov 2, 2021
Open
Open
Open
@kapouer
Copy link
Contributor

kapouer commented Nov 9, 2021

Could this go into 16.10.1 ?

@Trott
Copy link
Member

Trott commented Nov 9, 2021

@targos Should the lts-watch label go on #40684 instead, since that's the PR where this change was landed? This PR was closed without merging/landing.

@targos
Copy link
Member

targos commented Nov 9, 2021

Right, thanks.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@targos targos targos approved these changes

@tniessen tniessen tniessen approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@PiotrRybak @targos @nodejs-github-bot @tniessen @Trott @kapouer @lpinca @Mesteery