Skip to content

Conversation

jmaslak
Copy link

@jmaslak jmaslak commented Jan 26, 2025

1.1.1.2, as defined in the current code, runs a port 80 web server that responds to HTTP requests. Thus the timeout code is not doing a timeout test, but rather testing against a web server that doesn't understand the Arista API.

This PR fixes that, but also exposes what I documented in #61 - I.E. that a hardcoded 60 second timeout exists in Connect(), thus client_test.go unit tests now take 65s to run, thus it may be a good idea to address #61 so that timeout on connect is configurable as well.

Copy link
Contributor

@roopeshsn roopeshsn left a comment

Choose a reason for hiding this comment

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

I pulled your PR branch and ran the unit test.

roopesh:goeapi/ (pr-62) $ go test -run ^TestClientConnectTimeout_UnitTest$                                                                                           [15:07:41]
Connections:  [dut localhost]
PASS
ok      github.com/aristanetworks/goeapi        65.300s

Can't we change that hardcoded timeout value to be 30 seconds are lesser?

@jmaslak
Copy link
Author

jmaslak commented Apr 1, 2025

Yes, there are two things here:

  1. The hardcoded timeout (there would need to be a way to control that I think)

  2. The test wasn't actually testing a timeout, it was testing a connection refused. This PR fixes the second of these. I did not implement the fix for the first one.

@roopeshsn
Copy link
Contributor

Yes, there are two things here:

  1. The hardcoded timeout (there would need to be a way to control that I think)
  2. The test wasn't actually testing a timeout, it was testing a connection refused. This PR fixes the second of these. I did not implement the fix for the first one.

Agree that this test is not for testing the timeout. But 65s for a test case to run is huge in my opinion. I would highly suggest fixing issue 1 in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants