FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix cctest URLTest.ToFilePath on Win32 without Intl by jasnell · Pull Request #22265 · nodejs/node · GitHub

/ node Public

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

Closed
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:url-cctest-withoutintl
ConversationCommits (1)ChecksFiles changed
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 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 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 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 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 commented Aug 11, 2018

Copy link
Copy Markdown
Member Author

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

jasnell added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 12, 2018

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 reviewed Aug 13, 2018
View reviewed changes
Comment thread test/cctest/test_url.cc Outdated

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell commented Aug 14, 2018

Copy link
Copy Markdown
Member Author

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

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 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 commented Aug 15, 2018

Copy link
Copy Markdown
Member Author

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

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

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 commented Aug 15, 2018

Copy link
Copy Markdown
Member Author

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

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 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 commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

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

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 commented Aug 16, 2018

Copy link
Copy Markdown
Member

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

jasnell commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

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

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

jasnell commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

@TimothyGu ... else condition added, PTAL

jasnell commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

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

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
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 commented Aug 16, 2018

Copy link
Copy Markdown
Member Author

Landed in 26814cf

jasnell closed this Aug 16, 2018
joyeecheung mentioned this pull request Aug 17, 2018
Closed
targos pushed a commit that referenced this pull request Aug 19, 2018
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
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 mentioned this pull request Sep 5, 2018
Merged
This was referenced Sep 6, 2018
Closed
Closed
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 mentioned this pull request Aug 15, 2021
Open
molllyn1 mentioned this pull request Mar 22, 2024
Open
137717unity mentioned this pull request Mar 22, 2024
Open
kissofire mentioned this pull request Mar 22, 2024
Open
aliceUnhinged613 mentioned this pull request Mar 22, 2024
Open
ron-a20 mentioned this pull request Mar 22, 2024
Open
wedataintelligence mentioned this pull request Mar 23, 2024
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

joyeecheung joyeecheung approved these changes
richardlau richardlau approved these changes
BridgeAR BridgeAR approved these changes
+1 more reviewer
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 outstanding review comments, 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

Back | FazBrowse Home | New Git URL