Skip to content

Refactor getCommand to set stuff needed by pipe commands.#211

Merged
i4ki merged 2 commits intomasterfrom
fix/210
May 7, 2017
Merged

Refactor getCommand to set stuff needed by pipe commands.#211
i4ki merged 2 commits intomasterfrom
fix/210

Conversation

@vitorarins
Copy link
Copy Markdown
Member

@vitorarins vitorarins commented May 2, 2017

This is kind of shitty for now. But it fixes the problem.
The tests had to be refactored because of race conditions, but I don't know if it's the right solution.
Also, I had to leave the SetStdout call in executeCommand because cmd.StdoutPipe() is already setting Stdout 😞. Any ideas?

Fixes #210

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 2, 2017

Codecov Report

Merging #211 into master will increase coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #211      +/-   ##
=========================================
+ Coverage   56.29%   56.5%   +0.21%     
=========================================
  Files          24      25       +1     
  Lines        4036    4111      +75     
=========================================
+ Hits         2272    2323      +51     
- Misses       1520    1543      +23     
- Partials      244     245       +1
Impacted Files Coverage Δ
internal/sh/shell.go 69.07% <100%> (+0.02%) ⬆️
internal/sh/builtin/loader.go 100% <0%> (ø) ⬆️
internal/sh/builtin/glob.go 91.3% <0%> (ø)
nash.go 50.89% <0%> (+0.14%) ⬆️
internal/sh/builtin/print.go 87.5% <0%> (+1.78%) ⬆️
internal/sh/builtin/format.go 87.5% <0%> (+2.88%) ⬆️

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 d0ba359...9e94050. Read the comment docs.

}

cmd.SetStdin(shell.stdin)
cmd.SetStderr(shell.stderr)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As I'd said to you in person today, we can set stdout here also, but set it to nil before calling cmd.StdoutPipe in executePipe.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, will do that soon.

}

func TestExecutePipe(t *testing.T) {
var stderr bytes.Buffer
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what's the problem with the table tests?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ooh, because of the races we discussed in the other PR, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep! Any ideas?

Copy link
Copy Markdown
Collaborator

@i4ki i4ki left a comment

Choose a reason for hiding this comment

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

LGTM

@i4ki i4ki merged commit 241fd36 into master May 7, 2017
@i4ki i4ki removed the in progress label May 7, 2017
@i4ki i4ki deleted the fix/210 branch May 7, 2017 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

interpreter: pipe commands stuck with no output

3 participants