Re: shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile [try 6]

[Juan Lang]

Hi Ilya,
I like this one rather better, thanks.  Especially the use of broken()
makes it clearer what's happening.

I think you could tidy it up just a little more:
> - expected = 33; sprintf(fileA, testfile, tmpdir);
> - rc=shell_execute(NULL, fileA, NULL, NULL);
> - todo_wine {
> - ok(rc==expected || (rc>32 && expected>32),
> - "expected %s (%d), got %s (%d), lpFile: %s \n",
> - expected==33 ? "success" : "failure", expected, rc > 32 ? "success" : "failure", rc, fileA
> - );
> - }

In this case, expected is 33, so the "expected == 33" is unneeded.
Just replace "%s (%d)" with "success (32)".  Same below with the
remaining ok expressions:  make them match the actual value of
expected, rather than any possible value.  Otherwise, looks good to
me.

Thanks,
--Juan

shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile [try 6]

[Paul Vriens]

On 03/08/2010 05:42 PM, Juan Lang wrote:
> Hi Ilya,
> I like this one rather better, thanks.  Especially the use of broken()
> makes it clearer what's happening.
>
> I think you could tidy it up just a little more:
> > - expected = 33; sprintf(fileA, testfile, tmpdir);
> > - rc=shell_execute(NULL, fileA, NULL, NULL);
> > - todo_wine {
> > -
> >    ok(rc==expected || (rc>32&&  expected>32),
> > -
> >        "expected %s (%d), got %s (%d), lpFile: %s \n",
> > -
> >        expected==33 ? "success" : "failure", expected, rc>  32 ? "success" : "failure", rc, fileA
> > -
> >        );
> > - }
>
> In this case, expected is 33, so the "expected == 33" is unneeded.
> Just replace "%s (%d)" with "success (32)".  Same below with the
> remaining ok expressions:  make them match the actual value of
> expected, rather than any possible value.  Otherwise, looks good to
> me.

Yep, I like this one better as well. The only thing I see is that you 
have "==expected" and "rc>32" (which is both true in the above case). Is 
that to cover for OS differences? If so, please add some comments.

Re[2]: shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile [try 6]

[Ilya Basin]

PV> On 03/08/2010 05:42 PM, Juan Lang wrote:
> > Hi Ilya,
> > I like this one rather better, thanks.  Especially the use of broken()
> > makes it clearer what's happening.
> >
> > I think you could tidy it up just a little more:
> > > - expected = 33; sprintf(fileA, testfile, tmpdir);
> > > - rc=shell_execute(NULL, fileA, NULL, NULL);
> > > - todo_wine {
> > > -
> > >    ok(rc==expected || (rc>32&&  expected>32),
> > > -
> > >        "expected %s (%d), got %s (%d), lpFile: %s \n",
> > > -
> > >        expected==33 ? "success" : "failure", expected, rc>  32 ? "success" : "failure", rc, fileA
> > > -
> > >        );
> > > - }
> >
> > In this case, expected is 33, so the "expected == 33" is unneeded.
> > Just replace "%s (%d)" with "success (32)".  Same below with the
> > remaining ok expressions:  make them match the actual value of
> > expected, rather than any possible value.  Otherwise, looks good to
> > me.

PV> Yep, I like this one better as well. The only thing I see is that you 
PV> have "==expected" and "rc>32" (which is both true in the above case). Is 
PV> that to cover for OS differences? If so, please add some comments.
No, that's because I use the same expression for both failure and
success.
Now Juan tells me to have unique format string for each case. Let's stop
here already! I'm sure all of us are unable to predict what exactly
the guy with the commit power won't like in my patch. Quit taking
precautions, please.

Re[2]: shell32/tests: add tests for the parser of SHELLEXECUTEINFO.lpFile [try 6]

[Juan Lang]

> > > > - expected = 33; sprintf(fileA, testfile, tmpdir);
> > > > - rc=shell_execute(NULL, fileA, NULL, NULL);
> > > > - todo_wine {
> > > > - ok(rc==expected || (rc>32&&  expected>32),
> > > > - "expected %s (%d), got %s (%d), lpFile: %s \n",
> > > > - expected==33 ? "success" : "failure", expected, rc>  32 ? "success" : "failure", rc, fileA
> > > > - );
> > > > - }
> > >
> > > In this case, expected is 33, so the "expected == 33" is unneeded.
> > > Just replace "%s (%d)" with "success (32)".  Same below with the
> > > remaining ok expressions:  make them match the actual value of
> > > expected, rather than any possible value.  Otherwise, looks good to
> > > me.
>
> PV> Yep, I like this one better as well. The only thing I see is that you
> PV> have "==expected" and "rc>32" (which is both true in the above case). Is
> PV> that to cover for OS differences? If so, please add some comments.
> No, that's because I use the same expression for both failure and
> success.
> Now Juan tells me to have unique format string for each case. Let's stop
> here already!

We both have gotten many patches committed, so I think our predictions
are better than most ;-)

Paul and I don't disagree, actually, we just spotted different things.
 I was talking about the format string, he's talking about the ok
expression.  Both have unnecessary expressions, and we're both telling
you to remove what's unnecessary.  So, going back to the example we
started with, here's what you have:

+    expected = 33; sprintf(fileA, testfile, tmpdir);
+    rc=shell_execute(NULL, fileA, NULL, NULL);
+    todo_wine {
+        ok(rc==expected || (rc>32&&  expected>32),
+            "expected %s (%d), got %s (%d), lpFile: %s \n",
+            expected==33 ? "success" : "failure", expected, rc>  32 ?
"success" : "failure", rc, fileA
+            );
+    }

and here's what it would look like, taking into account both Paul's
and my suggestions:

+    expected = 33; sprintf(fileA, testfile, tmpdir);
+    rc=shell_execute(NULL, fileA, NULL, NULL);
+    todo_wine {
+        ok(rc==expected,
+            "expected success (33), got %s (%d), lpFile: %s \n",
+            rc>  32 ? "success" : "failure", rc, fileA
+            );
+    }

Hope that helps.
--Juan