Skip to content

cmd: check for errors on log websockets#533

Merged
bobheadxi merged 2 commits into
ubclaunchpad:masterfrom
didil:cmd/#526-client-log-websockets-error-check
Feb 7, 2019
Merged

cmd: check for errors on log websockets#533
bobheadxi merged 2 commits into
ubclaunchpad:masterfrom
didil:cmd/#526-client-log-websockets-error-check

Conversation

@didil

@didil didil commented Feb 7, 2019

Copy link
Copy Markdown
Contributor

🎟️ 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
View reviewed changes

@bobheadxi bobheadxi left a comment

Copy link
Copy Markdown
Member

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

Comment thread client/client.go Outdated
@codecov

codecov Bot commented Feb 7, 2019 •
edited
Loading

Copy link
Copy Markdown

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.

@bobheadxi @didil
more context with error message
01c8ccd 
Co-Authored-By: didil <1284255+didil@users.noreply.github.com>
bobheadxi
bobheadxi approved these changes Feb 7, 2019
View reviewed changes

@bobheadxi bobheadxi left a comment

Copy link
Copy Markdown
Member

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

bobheadxi commented Feb 7, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

@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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bobheadxi bobheadxi bobheadxi approved these changes

@kimoantiqe kimoantiqe Awaiting requested review from kimoantiqe

@seifghazi seifghazi Awaiting requested review from seifghazi

@terryz21 terryz21 Awaiting requested review from terryz21

@nicholaschinjie nicholaschinjie Awaiting requested review from nicholaschinjie

@yaoharry yaoharry Awaiting requested review from yaoharry

Assignees

No one assigned

Labels

pr: finalized needs review and final approval

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@didil @bobheadxi