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.

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

test: fixup failing test/internet/test-dns.js #38241

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 14, 2021

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]

@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 Apr 14, 2021
@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Apr 14, 2021
@nodejs-github-bot

This comment has been minimized.

@ycjcl868
Copy link
Contributor

CI broken

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 15, 2021

@jasnell
Copy link
Member Author

jasnell commented Apr 15, 2021

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

@jasnell jasnell added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 15, 2021
Copy link
Contributor

@RaisinTen RaisinTen left a 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:

// 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)

@richardlau
Copy link
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.

@jasnell
Copy link
Member Author

jasnell commented Apr 15, 2021

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

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
Copy link
Member Author

jasnell commented Apr 16, 2021

Landed in 5e588c1

@jasnell jasnell closed this Apr 16, 2021
jasnell added a commit that referenced this pull request Apr 17, 2021
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]>
targos pushed a commit that referenced this pull request May 1, 2021
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
danielleadams pushed a commit that referenced this pull request May 8, 2021
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
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants