Skip to content

🐛 TestDialWithBackoff work without special environment#2402

Merged
knative-prow[bot] merged 3 commits into
knative:mainfrom
cardil:bugfix/fix-TestDialWithBackoff
Apr 25, 2022
Merged

🐛 TestDialWithBackoff work without special environment#2402
knative-prow[bot] merged 3 commits into
knative:mainfrom
cardil:bugfix/fix-TestDialWithBackoff

Conversation

@cardil
Copy link
Copy Markdown
Contributor

@cardil cardil commented Jan 26, 2022

Changes

  • 🐛 TestDialWithBackoff work without special environment

/kind bug

Fixes #2401

@knative-prow-robot knative-prow-robot added kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 26, 2022
@cardil
Copy link
Copy Markdown
Contributor Author

cardil commented Jan 26, 2022

/cc @vagababov
/cc @tcnghia
/cc @mattmoor

@knative-prow-robot
Copy link
Copy Markdown
Contributor

@cardil: GitHub didn't allow me to request PR reviews from the following users: Chizh.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @vagababov
/cc @tcnghia
/cc @Chizh
/cc @mattmoor

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@cardil
Copy link
Copy Markdown
Contributor Author

cardil commented Jan 26, 2022

/cc @chizhg

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 26, 2022

Codecov Report

Merging #2402 (abf66bb) into main (12be060) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2402   +/-   ##
=======================================
  Coverage   81.71%   81.71%           
=======================================
  Files         163      163           
  Lines        9653     9653           
=======================================
  Hits         7888     7888           
  Misses       1529     1529           
  Partials      236      236           
Impacted Files Coverage Δ
network/transports.go 71.95% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12be060...abf66bb. Read the comment docs.

@cardil
Copy link
Copy Markdown
Contributor Author

cardil commented Jan 26, 2022

The unit test failures 1486341657388388352 look unrelated:

/test pull-knative-pkg-unit-tests

@chizhg
Copy link
Copy Markdown
Contributor

chizhg commented Jan 28, 2022

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2022
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cardil, chizhg
To complete the pull request process, please assign tcnghia after the PR has been reviewed.
You can assign the PR to them by writing /assign @tcnghia in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Comment thread network/transports_test.go
Comment thread network/transports_test.go Outdated
Comment thread network/transports_test.go Outdated
Comment thread network/transports.go
Comment thread network/transports_test.go Outdated
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2022
@cardil cardil force-pushed the bugfix/fix-TestDialWithBackoff branch from fbf91d7 to 15ba2b8 Compare April 22, 2022 18:49
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 22, 2022
@knative-prow knative-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 22, 2022
@evankanderson
Copy link
Copy Markdown
Member

Rather than using a likely-non-routable IP, how about using Listen's backlog to force a timeout? (This is similar to the localhost IP range suggestion, but more likely to consistently work)

cardil#1

Note that both this PR and the current state in main don't work on Windows, with similar errors:

--- FAIL: TestDialWithBackoff (0.17s)
    --- FAIL: TestDialWithBackoff/ConnectionRefused (0.09s)
        transports_test.go:107: Unexpected error: timed out dialing after 0.09s
--- FAIL: TestDialTLSWithBackoff (0.19s)
    --- FAIL: TestDialTLSWithBackoff/ConnectionRefused (0.09s)
        transports_test.go:107: Unexpected error: timed out dialing after 0.09s

As a side note, putting all of these in one test makes it harder (for example) to run just the DialWithBackoffTimout test from VSCode -- you can click on "run test" above methods named TestFoo to run just that test, but in this case, you have to run the TestDialWithBackoff tests as a whole.

@cardil
Copy link
Copy Markdown
Contributor Author

cardil commented Apr 22, 2022

At @evankanderson:

Rather than using a likely-non-routable IP, how about using Listen's backlog to force a timeout? cardil#1

Interesting. I'll continue on this next week.

Note that both this PR and the current state in main don't work on Windows, with similar errors

I did run those tests multiple times, but apparently they also failed on Prow. It looks to me like your solution could help. Thanks! I'll take a look next week.

As a side note, putting all of these in one test makes it harder to run just one test

That's a bummer. I use Goland, and there I could easily execute those subtests. VSCode should also have this feature.

Screenshot from 2022-04-22 22-40-59

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 25, 2022
@knative-prow
Copy link
Copy Markdown

knative-prow Bot commented Apr 25, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, chizhg, evankanderson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2022
@knative-prow knative-prow Bot merged commit 45c37c2 into knative:main Apr 25, 2022
@cardil cardil deleted the bugfix/fix-TestDialWithBackoff branch April 26, 2022 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TestDialWithBackoff from network/transports_test.go fails by not recognizing expected error outside of CI

5 participants