GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: fixup failing test/internet/test-dns.js by jasnell · Pull Request #38241 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Apr 14, 2021

Copy link
Copy Markdown
Member

The ttl for the nodejs.org DNS record is returning 0 currently. The test checks for > 0, causing the test to fail.

Signed-off-by: James M Snell [email protected]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to Node.js core tests and test infrastructure. labels Apr 14, 2021
addaleax
addaleax approved these changes Apr 14, 2021
View reviewed changes
richardlau
richardlau approved these changes Apr 14, 2021
View reviewed changes
@jasnell jasnell added author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed needs-ci PRs that need a full CI run. labels Apr 14, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
marsonya
marsonya approved these changes Apr 15, 2021
View reviewed changes
ycjcl868
ycjcl868 approved these changes Apr 15, 2021
View reviewed changes
@ycjcl868

ycjcl868 commented Apr 15, 2021

Copy link
Copy Markdown
Contributor

CI broken
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Apr 15, 2021 •
edited by jasnell
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/37483/ 💚
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 15, 2021

Copy link
Copy Markdown
Member Author

CI is green, there's no reason to make this wait the full 48 hours. Please 👍🏻 to fast-track
Linkgoron reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the fast-track PRs proposed for a shorter-than-standard waiting period before landing. label Apr 15, 2021
RaisinTen
RaisinTen approved these changes Apr 15, 2021
View reviewed changes

@RaisinTen RaisinTen left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@jasnell Should we update this comment here as well:

node/test/parallel/test-dgram-multicast-setTTL.js

Line 33 in 82d5988

// Try to set an invalid TTL (valid ttl is > 0 and < 256) 

-  // Try to set an invalid TTL (valid ttl is > 0 and < 256)
+  // Try to set an invalid TTL (valid ttl is >= 0 and < 256)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@richardlau

richardlau commented Apr 15, 2021

Copy link
Copy Markdown
Member

CI is green, there's no reason to make this wait the full 48 hours. Please 👍🏻 to fast-track

There's the known failing Build from tarball action. Our collaborator guide says
https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md#testing-and-ci

A passing (green) GitHub Actions CI result is required.

You could argue the failing test is nothing to do with this PR, but if collaborators are free to ignore failing CI we might as well not bother running them.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 15, 2021

Copy link
Copy Markdown
Member Author

Ugh... I missed that failing github ci bit...extremely frustrating.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
VoltrexKeyva
VoltrexKeyva approved these changes Apr 15, 2021
View reviewed changes
@jasnell
test: fixup failing test/internet/test-dns.js
312ad3a 
The `ttl` for the `nodejs.org` DNS record is returning `0`
currently. The test checks for `> 0`, causing the test to
fail.

Signed-off-by: James M Snell <[email protected]>
@jasnell
jasnell force-pushed the fixup-internet-test-dns branch from 0d92399 to 312ad3a Compare April 16, 2021 16:05
lpinca
lpinca approved these changes Apr 16, 2021
View reviewed changes
@jasnell

jasnell commented Apr 16, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Landed in 5e588c1
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Apr 16, 2021
jasnell added a commit that referenced this pull request Apr 17, 2021
@jasnell
test: fixup failing test/internet/test-dns.js
5e588c1 
The `ttl` for the `nodejs.org` DNS record is returning `0`
currently. The test checks for `> 0`, causing the test to
fail.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #38241
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 20, 2021
2021-04-20, Version 16.0.0 (Current) #37678
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@jasnell @targos
test: fixup failing test/internet/test-dns.js
ea1183c 
The `ttl` for the `nodejs.org` DNS record is returning `0`
currently. The test checks for `> 0`, causing the test to
fail.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #38241
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
danielleadams pushed a commit that referenced this pull request May 8, 2021
@jasnell @danielleadams
test: fixup failing test/internet/test-dns.js
c659034 
The `ttl` for the `nodejs.org` DNS record is returning `0`
currently. The test checks for `> 0`, causing the test to
fail.

Signed-off-by: James M Snell <[email protected]>

PR-URL: #38241
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@lpinca lpinca lpinca approved these changes
@richardlau richardlau richardlau approved these changes
@marsonya marsonya marsonya approved these changes
@RaisinTen RaisinTen RaisinTen approved these changes
+2 more reviewers
@ycjcl868 ycjcl868 ycjcl868 approved these changes
@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. fast-track PRs proposed for a shorter-than-standard waiting period before landing. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@jasnell @nodejs-github-bot @ycjcl868 @richardlau @addaleax @lpinca @marsonya @RaisinTen @VoltrexKeyva
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.