-
Notifications
You must be signed in to change notification settings - Fork 50
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
add tests for test package #75
Conversation
also: • fix ExitStatus for test package • Allow POST for test package
2ef3eae
to
4727578
Compare
@@ -119,7 +119,7 @@ func MockFunc(e mockEnvironment) net.Conn { | |||
for { | |||
d, err := readPbFrame(conB) | |||
if err != nil { | |||
e.Errorf("Can't read method name") | |||
// Conn probably closed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting an error with go test -race trying to access e from this goroutine. Open to ideas how to fix this better, but it got my tests working
if req.Body != "" { | ||
return fmt.Errorf("GET requests must not have body, found \"%v\"", req.Body) | ||
} | ||
return nil | ||
case "POST": | ||
return nil | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to only whitelist certain methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like unfinished code.
@@ -442,9 +446,9 @@ func (e *TestEnv) Handle(method string, args_d []byte) []byte { | |||
args := kong_plugin_protocol.ExitArgs{} | |||
e.noErr(proto.Unmarshal(args_d, &args)) | |||
e.ClientRes.Status = int(args.Status) | |||
e.state = finished |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExitStatus was not finishing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack for this addition
@fffonion please review |
Hi. @jgramoll Do you still want to push this forward? Please rebase the branch and solve the conflict. |
Opened #196 picking the changes here. |
also:
• fix ExitStatus for test package
• Allow POST for test package