Skip to content

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

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:url-cctest-withoutintl
Closed

test: fix cctest URLTest.ToFilePath on Win32 without Intl#22265
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:url-cctest-withoutintl

Conversation

@jasnell

@jasnell jasnell commented Aug 11, 2018

Copy link
Copy Markdown
Member

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

nodejs-github-bot commented Aug 11, 2018

Copy link
Copy Markdown
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

jasnell commented Aug 11, 2018

Copy link
Copy Markdown
Member Author

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

jasnell commented Aug 13, 2018

Copy link
Copy Markdown
Member Author

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

TimothyGu
TimothyGu reviewed Aug 13, 2018
View reviewed changes
Comment thread test/cctest/test_url.cc Outdated

@TimothyGu TimothyGu Aug 13, 2018

Copy link
Copy Markdown
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

jasnell commented Aug 14, 2018

Copy link
Copy Markdown
Member Author

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

@jasnell

jasnell commented Aug 15, 2018

Copy link
Copy Markdown
Member Author

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

maclover7 commented Aug 15, 2018

Copy link
Copy Markdown
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

jasnell commented Aug 15, 2018

Copy link
Copy Markdown
Member Author

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

@joyeecheung

joyeecheung commented Aug 15, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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

@joyeecheung

joyeecheung commented Aug 15, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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 jasnell force-pushed the url-cctest-withoutintl branch from 7ccbcca to fd4cbcd Compare August 15, 2018 16:08
@jasnell

jasnell commented Aug 15, 2018

Copy link
Copy Markdown
Member Author

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

@jasnell

jasnell commented Aug 15, 2018

Copy link
Copy Markdown
Member Author

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

@jasnell

jasnell commented Aug 15, 2018

Copy link
Copy Markdown
Member Author

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

Trott commented Aug 15, 2018

Copy link
Copy Markdown
Member

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

jasnell commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

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

@jasnell

jasnell commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

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

@TimothyGu

TimothyGu commented Aug 16, 2018

Copy link
Copy Markdown
Member

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

@jasnell

jasnell commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

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

@jasnell jasnell force-pushed the url-cctest-withoutintl branch from fd4cbcd to afa6c76 Compare August 16, 2018 21:12
@jasnell

jasnell commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

@TimothyGu ... else condition added, PTAL

@jasnell

jasnell commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

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 jasnell force-pushed the url-cctest-withoutintl branch from afa6c76 to a86a5f6 Compare August 16, 2018 22:14
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

jasnell commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

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

@joyeecheung joyeecheung joyeecheung approved these changes

@richardlau richardlau richardlau approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

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