Re: [PATCH 1/9] cmd/tests: add tests for command line parsing

[Frédéric Delanoy]

On Tue, Sep 20, 2011 at 01:51, Martin Wilck <mwilck(a)arcor.de> wrote:
> This patch adds some tests for command line parsing and treatment
> of quotes and special characters in the command line and in program
> names and parameters. The .exp file was created using Win XP.
> ---
>  programs/cmd/tests/rsrc.rc              |    6 +
>  programs/cmd/tests/test_cmdline.cmd     |  159 ++++++++++++++++++++++
>  programs/cmd/tests/test_cmdline.cmd.exp |  219 +++++++++++++++++++++++++++++++
>  3 files changed, 384 insertions(+), 0 deletions(-)
>  create mode 100755 programs/cmd/tests/test_cmdline.cmd
>  create mode 100755 programs/cmd/tests/test_cmdline.cmd.exp

You might want to do a single "@echo off" at the start of the .cmd file.
Echoing every command in the .exp isn't very helpful: you don't want
to replicate the .cmd contents in the .exp file. Add comments or
"subsections" as is done in test_builtins.cmd if necessary, or even
better use descriptive results, like e.g. 'text in single quotes' if
possible
(Also the @ before every rem or echo just add visual clutter IMHO but
that's due to the enabled echo mode)

[PATCH 1/9] cmd/tests: add tests for command line parsing

[Frédéric Delanoy]

2011/9/20 Frédéric Delanoy <frederic.delanoy(a)gmail.com>:
> On Tue, Sep 20, 2011 at 01:51, Martin Wilck <mwilck(a)arcor.de> wrote:
> > This patch adds some tests for command line parsing and treatment
> > of quotes and special characters in the command line and in program
> > names and parameters. The .exp file was created using Win XP.
> > ---
> >  programs/cmd/tests/rsrc.rc              |    6 +
> >  programs/cmd/tests/test_cmdline.cmd     |  159 ++++++++++++++++++++++
> >  programs/cmd/tests/test_cmdline.cmd.exp |  219 +++++++++++++++++++++++++++++++
> >  3 files changed, 384 insertions(+), 0 deletions(-)
> >  create mode 100755 programs/cmd/tests/test_cmdline.cmd
> >  create mode 100755 programs/cmd/tests/test_cmdline.cmd.exp
>
> You might want to do a single "@echo off" at the start of the .cmd file.
> Echoing every command in the .exp isn't very helpful: you don't want
> to replicate the .cmd contents in the .exp file. Add comments or
> "subsections" as is done in test_builtins.cmd if necessary, or even
> better use descriptive results, like e.g. 'text in single quotes' if
> possible
> (Also the @ before every rem or echo just add visual clutter IMHO but
> that's due to the enabled echo mode)

Also, there should be no trailing whitespace.
See http://wiki.winehq.org/SubmittingPatches

[PATCH 1/9] cmd/tests: add tests for command line parsing

[Martin Wilck]

On 09/20/2011 01:24 AM, Frédéric Delanoy wrote:
> You might want to do a single "@echo off" at the start of the .cmd file.
> Echoing every command in the .exp isn't very helpful: you don't want
> to replicate the .cmd contents in the .exp file.

I did that on purpose. It makes figuring out problems *way* easier. I
hate counting lines in the .exp file just to see which test was failing.
However, if that's a blocking point for getting the patches applied, it
will be no problem to change that.

Martin

[PATCH 1/9] cmd/tests: add tests for command line parsing

[Frédéric Delanoy]

2011/9/20 Martin Wilck <mwilck(a)arcor.de>:
> On 09/20/2011 01:24 AM, Frédéric Delanoy wrote:
> > You might want to do a single "@echo off" at the start of the .cmd file.
> > Echoing every command in the .exp isn't very helpful: you don't want
> > to replicate the .cmd contents in the .exp file.
>
> I did that on purpose. It makes figuring out problems *way* easier. I
> hate counting lines in the .exp file just to see which test was failing.

You don't need counting lines. Just put appropriate self-descriptive
messages + short test (sub)sections  Use echoing only for difficult
cases.
A shorter patch is easier to review and more auto-documenting.
The .exp file is not there for replicating the .cmd file, otherwise
the .cmd file wouldn't be needed in the first place...

Ex: (testing echo command here, but valid for all other commands)
(cmd)
@echo off [this line is generally not needed, unless echo mode was
previously activated]
echo ... quoted strings ...
echo 'single-quoted string'
echo "double-quoted string"
echo `backquoted string`
echo ... another subsection ...
...

(exp)
... quoted strings...
'single-quoted string'
"double-quoted string"
`backquoted string`
... another subsection ...
> However, if that's a blocking point for getting the patches applied, it
> will be no problem to change that.

I'm not the one who commits patches, but they should be as concise as possible.
See http://wiki.winehq.org/SubmittingPatches