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

test: fix cctest URLTest.ToFilePath on Win32 without Intl #22265

Closed
wants to merge 1 commit into from
Closed

test: fix cctest URLTest.ToFilePath on Win32 without Intl #22265

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 11, 2018

Skip one specific test that requires ICU

Fixes: #19051

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@jasnell jasnell added c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 11, 2018
@nodejs-github-bot
Copy link
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/519/pipeline

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 11, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 11, 2018

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

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 12, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 13, 2018

Re-run CI on freebsd: https://ci.nodejs.org/job/node-test-commit-freebsd/19632/

TimothyGu
TimothyGu reviewed Aug 13, 2018
View reviewed changes
test/cctest/test_url.cc Outdated
T("file://xn--weird-prdj8vva.com/host/a", "\\\\wͪ͊eiͬ͋rd.com\\host\\a");
#endif
Copy link
Member

Choose a reason for hiding this comment

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

It might be useful to have an #else testing what it actually is?

@jasnell
Copy link
Member Author

jasnell commented Aug 14, 2018

Freebsd again: https://ci.nodejs.org/job/node-test-commit-freebsd/19667/

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

Another day, another attempt to get a clean freebsd test run: https://ci.nodejs.org/job/node-test-commit-freebsd/19688/

/cc @nodejs/build

@maclover7
Copy link
Contributor

Another day, another attempt to get a clean freebsd test run

@jasnell I'll take a look at the machines once the security releases are out

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

Ok. yeah, the freebsd run failed again. :-(

@joyeecheung
Copy link
Member

joyeecheung commented Aug 15, 2018 •
edited
Loading

Somehow this is not rebased onto any commit later than 4ce744a (which "fixes" the stringbytes test on freebsd) even though it should

@joyeecheung
Copy link
Member

joyeecheung commented Aug 15, 2018 •
edited
Loading

Oh, right @jasnell did you start the freebsd build with a older sha to rebase onto? It should be gone if you rebase onto the current master's HEAD. We can wait until the security release is out

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

Yep, looks like it wasn't rebased. Rebased now and pushed... will try it again.

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 15, 2018

Trying again on linux-openssl110 ... https://ci.nodejs.org/job/node-test-commit-linux-containered/6291/

this one appears to be broken in general.

@Trott
Copy link
Member

Trott commented Aug 15, 2018

Trying again on linux-openssl110 ... https://ci.nodejs.org/job/node-test-commit-linux-containered/6291/

this one appears to be broken in general.

Might be related to today's release. (Shared libs OpenSSL needs or needed to be updated to accommodate the updated OpenSSL in today's release.) Might be best to wait for the release to be out, rebase, and try again. @nodejs/build-infra

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

linux-containered yet again: https://ci.nodejs.org/job/node-test-commit-linux-containered/6333/

@TimothyGu
Copy link
Member

@jasnell Just making sure this doesn't get lost… #22265 (comment)

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

That did get lost, ok, will add a condition there.

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

@TimothyGu ... else condition added, PTAL

@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

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

@jasnell
test: fix cctest URLTest.ToFilePath on Win32 without Intl
a86a5f6 
Skip one specific test that requires ICU

Fixes: nodejs#19051
jasnell added a commit that referenced this pull request Aug 16, 2018
@jasnell
test: fix cctest URLTest.ToFilePath on Win32 without Intl
26814cf 
Skip one specific test that requires ICU

Fixes: #19051

PR-URL: #22265
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jasnell
Copy link
Member Author

jasnell commented Aug 16, 2018

Landed in 26814cf

@jasnell jasnell closed this Aug 16, 2018
@joyeecheung joyeecheung mentioned this pull request Aug 17, 2018
Closed
targos pushed a commit that referenced this pull request Aug 19, 2018
@jasnell @targos
test: fix cctest URLTest.ToFilePath on Win32 without Intl
e515ce0 
Skip one specific test that requires ICU

Fixes: #19051

PR-URL: #22265
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Sep 3, 2018
@jasnell @targos
test: fix cctest URLTest.ToFilePath on Win32 without Intl
44bcc1d 
Skip one specific test that requires ICU

Fixes: #19051

PR-URL: #22265
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos mentioned this pull request Sep 5, 2018
Merged
This was referenced Sep 6, 2018
Closed
Closed
@snyk-bot snyk-bot mentioned this pull request Aug 11, 2020
Open
This was referenced Aug 5, 2021
Open
Open
Open
Open
Open
Open
Open
Open
@snyk-bot snyk-bot mentioned this pull request Aug 15, 2021
Open
@molllyn1 molllyn1 mentioned this pull request Mar 22, 2024
Open
@137717unity 137717unity mentioned this pull request Mar 22, 2024
Open
@kissofire kissofire mentioned this pull request Mar 22, 2024
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Mar 22, 2024
Open
@ron-a20 ron-a20 mentioned this pull request Mar 22, 2024
Open
@wedataintelligence wedataintelligence mentioned this pull request Mar 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@richardlau richardlau richardlau approved these changes

@BridgeAR BridgeAR BridgeAR 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. c++ Issues and PRs that require attention from people who are familiar with C++. test Issues and PRs related to the tests. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jasnell @nodejs-github-bot @maclover7 @joyeecheung @Trott @TimothyGu @richardlau @BridgeAR