| Message ID | 20220222114313.14921-1-shivam828787@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | microproject: avoid using pipes in test | expand |
On 2/22/2022 6:43 AM, Shubham Mishra wrote: Welcome, Subham! > pipes doesn't care about error codes and ignore them thus we should not use them in tests. > As an easy alternative, I am using a tmp file to write from git command so we can test the exit code. This is the correct way to convert the pipes into something that properly notices Git failures. The only issue I have with your patches is that you should insert a newline after your &&. (I'll include an example on patch 1.) > This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files. I think this is a great start! After fixing the formatting that I saw, you should be good to start making more changes in a single series. These patches are a good size. Thanks, -Stolee
On Tue, Feb 22 2022, Shubham Mishra wrote:
> pipes doesn't care about error codes and ignore them thus we should not use them in tests.
Aside from what Derrick Stolee mentioned in his feedback, all of which I
agree with.
I think these changes are good, but it's not the case that we try to
avoid using pipes at all in our tests.
It's often a hassle, and just not worth it, e.g.:
oid=$(echo foo | git hash-object --stdin -w) &&
Sure, we can make that:
echo foo >in &&
oid=$(git hash-object --stdin -w <in) &&
But in the general case it's not worth worrying about.
What we *do* try to avoid, and what's actually important is to never
invoke "git" or other programs we invoke on the LHS of a pipe, or to
otherwise do so in a way that hides potential errors.
That's not isolated to just pipes, but e.g. calling it within helper
functions that don't &&-chain, but pipes are probably the most common
offender.
The reason we do that is because in hacking git we may make it error,
segfault etc. If it's on the LHS of a pipe that failure becomes
indistinguishable from success.
And if the test is really checking e.g. "when I run git like this, it
produces no output" printing nothing with an exit of 0 will become the
same as a segafault for the purposes of test.
And that's bad.
But just invoking things on the LHS of a pipe? Sometimes it's a good
thing not do do that, because we'll be able to see a failure more
incrementally, and with intermediate files.
But it's generally not a problem, our test suite assumes that the OS is
basically sane. We're not going to call every invocation of "sed",
"grep" etc. with a wrapper like "test_must_fail grep" instead of "!
grep".
The same goes for our own helper utility functions such as "q_to_nul"
etc, as long as (and this is critical) that they're implemented by
shelling out to "sed", "grep", "perl" or whatever, and not to "git" or
"test-tool" etc. Then we need to start being as paranoid about them as
"git" on the LHS of pipes.
> I think these changes are good, but it's not the case that we try to > avoid using pipes at all in our tests. > > It's often a hassle, and just not worth it, e.g.: > > oid=$(echo foo | git hash-object --stdin -w) && > > Sure, we can make that: > > echo foo >in && > oid=$(git hash-object --stdin -w <in) && > > But in the general case it's not worth worrying about. > > What we *do* try to avoid, and what's actually important is to never > invoke "git" or other programs we invoke on the LHS of a pipe, or to > otherwise do so in a way that hides potential errors. Sorry, I have not mentioned it properly in the message but my intention is not to remove all pipes but to remove only those, which have "git" command in LHS. > That's not isolated to just pipes, but e.g. calling it within helper > functions that don't &&-chain, but pipes are probably the most common > offender. > > The reason we do that is because in hacking git we may make it error, > segfault etc. If it's on the LHS of a pipe that failure becomes > indistinguishable from success. > > And if the test is really checking e.g. "when I run git like this, it > produces no output" printing nothing with an exit of 0 will become the > same as a segafault for the purposes of test. > > And that's bad. > > But just invoking things on the LHS of a pipe? Sometimes it's a good > thing not do do that, because we'll be able to see a failure more > incrementally, and with intermediate files. > > But it's generally not a problem, our test suite assumes that the OS is > basically sane. We're not going to call every invocation of "sed", > "grep" etc. with a wrapper like "test_must_fail grep" instead of "! > grep". > > The same goes for our own helper utility functions such as "q_to_nul" > etc, as long as (and this is critical) that they're implemented by > shelling out to "sed", "grep", "perl" or whatever, and not to "git" or > "test-tool" etc. Then we need to start being as paranoid about them as > "git" on the LHS of pipes. Thanks here for providing me with a broader context of the problem. What I understand, It's not just about "git" on LHS of pipes but more broader to anything custom where We can miss exit codes but I think as a low hanging fruit I can start with "git" on LHS of pipe and as I will understand the codebase more I can work on other custom helpers too. Thanks, Shubham
On Tue, Feb 22, 2022 at 03:24:31PM +0100, Ævar Arnfjörð Bjarmason wrote: > > On Tue, Feb 22 2022, Shubham Mishra wrote: > > > pipes doesn't care about error codes and ignore them thus we should not use them in tests. > > Aside from what Derrick Stolee mentioned in his feedback, all of which I > agree with. > > I think these changes are good, but it's not the case that we try to > avoid using pipes at all in our tests. > > It's often a hassle, and just not worth it, e.g.: > > oid=$(echo foo | git hash-object --stdin -w) && > > Sure, we can make that: > > echo foo >in && > oid=$(git hash-object --stdin -w <in) && > > But in the general case it's not worth worrying about. Agreed, and I would add that we don't necessarily need to worry about non-Git commands on the left-hand side of a pipe. So something like: find ... | sort >actual isn't a problem for us, because our test suite assumes that something like find will not fail. So leaving instances of those alone is OK, but... > What we *do* try to avoid, and what's actually important is to never > invoke "git" or other programs we invoke on the LHS of a pipe, or to > otherwise do so in a way that hides potential errors. > > That's not isolated to just pipes, but e.g. calling it within helper > functions that don't &&-chain, but pipes are probably the most common > offender. > > The reason we do that is because in hacking git we may make it error, > segfault etc. If it's on the LHS of a pipe that failure becomes > indistinguishable from success. > > And if the test is really checking e.g. "when I run git like this, it > produces no output" printing nothing with an exit of 0 will become the > same as a segafault for the purposes of test. ...yes, we do care about Git failures. So something like: git ls-files | grep "want" would be no-good, since any failures running 'git ls-files' would be quashed by the pipe. Thanks, Taylor
On Tue, Feb 22, 2022 at 6:01 PM Shubham Mishra <shivam828787@gmail.com> wrote: > > pipes doesn't care about error codes and ignore them thus we should not use them in tests. > As an easy alternative, I am using a tmp file to write from git command so we can test the exit code. (Others have already rightly commented on many things, so I am only focusing on microproject related things.) As the subject of this cover letter starts with "microproject:" I would suggest taking a look (or maybe reading again) our page about microprojects: https://git.github.io/General-Microproject-Information/ Please note that we ask that all related emails start with “[GSoC]” or “[Outreachy]” or something similar, not "microproject:". > This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files. Please read the "Only ONE quality focused microproject per student" section of the above mentioned page. Thanks for your first contribution!
> (Others have already rightly commented on many things, so I am only > focusing on microproject related things.) > > As the subject of this cover letter starts with "microproject:" I > would suggest taking a look (or maybe reading again) our page about > microprojects: https://git.github.io/General-Microproject-Information/ > > Please note that we ask that all related emails start with “[GSoC]” or > “[Outreachy]” or something similar, not "microproject:". Ack. > > This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files. > > Please read the "Only ONE quality focused microproject per student" > section of the above mentioned page. here I mean in next patches, I will be fixing the same "Git on LHS of pipe" for other tests. I think that will be considered as the same microproject and I am still eligible to fix the same thing for other files? Thanks, Shubham
On Thu, Feb 24, 2022 at 6:22 AM Shubham Mishra <shivam828787@gmail.com> wrote: > > > This is my first contribution that's why I am keeping diff short with an intention to understand the process instead of making impactful change in first attempt. Later, I will send more patches to fix the issue for other files. > > > > Please read the "Only ONE quality focused microproject per student" > > section of the above mentioned page. > > here I mean in next patches, I will be fixing the same "Git on LHS of > pipe" for other tests. I think that will be considered as the same > microproject and I am still eligible to fix the same thing for other > files? In the "Only ONE quality focused microproject per student" section, there is: => This means that for a microproject that consist in refactoring or rewriting a small amount of code, your patch should change only ONE file, or perhaps 2 files if they are closely related, like “foo.c” and “foo.h”. So, no, we really prefer you to focus on 1 file (or 2 files when they are strongly related) and then move on to your application or regular patches, reviews, discussions, etc.
> So, no, we really prefer you to focus on 1 file (or 2 files when they > are strongly related) and then move on to your application or regular > patches, reviews, discussions, etc. I misunderstood this :(. I will not submit any further patches related to microprojects but I already sentone patch at Morning - https://lore.kernel.org/git/20220224054720.23996-1-shivam828787@gmail.com. Can I finish this?. Thank you for clarifying. Thanks, Shubham.
On Thu, Feb 24, 2022 at 11:13 AM Shubham Mishra <shivam828787@gmail.com> wrote: > > > So, no, we really prefer you to focus on 1 file (or 2 files when they > > are strongly related) and then move on to your application or regular > > patches, reviews, discussions, etc. > > I misunderstood this :(. I will not submit any further patches related > to microprojects but I already sentone patch at Morning - > https://lore.kernel.org/git/20220224054720.23996-1-shivam828787@gmail.com. > Can I finish this?. Yeah, you can finish it. As I said to another applicant, we are even likely to help you finish it. Best, Christian.