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

cmd: check for errors on log websockets #533

Conversation

didil
Copy link
Contributor

@didil didil commented Feb 7, 2019

🎟️ Ticket(s): Closes #526


πŸ‘· Changes

add error checking on client.LogsWebSocket + test

πŸ”¦ Testing Instructions

make test

@didil didil requested a review from bobheadxi as a code owner February 7, 2019 03:29
@bobheadxi bobheadxi added the pr: finalized needs review and final approval label Feb 7, 2019
bobheadxi
bobheadxi previously approved these changes Feb 7, 2019
Copy link
Member

@bobheadxi bobheadxi left a comment

Choose a reason for hiding this comment

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

looks good, thanks! Just one minor comment

client/client.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #533 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
+ Coverage   56.22%   56.25%   +0.03%     
==========================================
  Files          60       60              
  Lines        2983     2985       +2     
==========================================
+ Hits         1677     1679       +2     
  Misses       1098     1098              
  Partials      208      208
Impacted Files Coverage Ξ”
client/client.go 70.72% <100%> (+0.25%) ⬆️

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 8e86582...01c8ccd. Read the comment docs.

Co-Authored-By: didil <1284255+didil@users.noreply.github.com>
Copy link
Member

@bobheadxi bobheadxi left a comment

Choose a reason for hiding this comment

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

thank! πŸŽ‰

@bobheadxi bobheadxi merged commit 026ecae into ubclaunchpad:master Feb 7, 2019
@bobheadxi
Copy link
Member

bobheadxi commented Feb 7, 2019

@didil looks like Windows returns a different error when the websocket connection fails:
https://ci.appveyor.com/project/ubclaunchpad/inertia/builds/22186666

--- FAIL: TestLogsWebsocketNoDaemon (1.02s)
    client_test.go:378: 
        	Error Trace:	client_test.go:378
        	Error:      	"failed to connect to daemon at 127.0.0.1:1150: dial tcp 127.0.0.1:1150: connectex: No connection could be made because the target machine actively refused it." does not contain "connect: connection refused"
        	Test:       	TestLogsWebsocketNoDaemon
FAIL
coverage: 55.6% of statements
FAIL	github.com/ubclaunchpad/inertia/client	3.164s

unfortunately I can't seem to get Appveyor to trigger on our end for forked repositories, so a Windows test run was never made for this PR, and the one for master just failed. Do you mind putting in a PR to address this?

@didil didil mentioned this pull request Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: finalized needs review and final approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants