Issue 8365032: Process tests work across platforms.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+72 lines, -27 lines)			Patch
M	tests/standalone/src/ProcessExitTest.dart	View	1 2 3	1 chunk	+3 lines, -1 line	0 comments	Download
M	tests/standalone/src/ProcessSegfaultTest.dart	View	1 2 3	1 chunk	+3 lines, -1 line	0 comments	Download
M	tests/standalone/src/ProcessStderrTest.dart	View	1 2 3 4 5	3 chunks	+24 lines, -9 lines	0 comments	Download
M	tests/standalone/src/ProcessStdoutTest.dart	View	1 2 3 4 5	3 chunks	+24 lines, -10 lines	0 comments	Download
A	tests/standalone/src/ProcessTestUtil.dart	View	1 2 3	1 chunk	+18 lines, -0 lines	0 comments	Download
M	tests/standalone/standalone.status	View	1 2 3 4 5	1 chunk	+0 lines, -6 lines	0 comments	Download

Messages

Total messages: 11 (0 generated)

Expand Messages | Collapse Messages

Ivan Posva

LGTM after fixing the status file. -Ivan http://codereview.chromium.org/8365032/diff/3002/tests/standalone/standalone.status File tests/standalone/standalone.status (right): http://codereview.chromium.org/8365032/diff/3002/tests/standalone/standalone.status#newcode25 tests/standalone/standalone.status:25: Process*: Skip ...

9 years, 2 months ago (2011-10-21 15:42:29 UTC) #3

Søren Gjesse

http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/ProcessStderrTest.dart File tests/standalone/src/ProcessStderrTest.dart (right): http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/ProcessStderrTest.dart#newcode36 tests/standalone/src/ProcessStderrTest.dart:36: for (int i = 0; i < BUFFERSIZE; i++) ...

9 years, 2 months ago (2011-10-24 06:23:04 UTC) #4

jrgfogh

9 years, 2 months ago (2011-10-24 09:03:21 UTC) #5

Ivan Posva

What is the status of this change? Any plan on committing this, if not please ...

9 years, 1 month ago (2011-11-02 16:42:41 UTC) #6

Søren Gjesse

On 2011/11/02 16:42:41, Ivan Posva wrote: > What is the status of this change? Any ...

9 years, 1 month ago (2011-11-03 12:37:09 UTC) #7

On 2011/11/02 16:42:41, Ivan Posva wrote:
> What is the status of this change? Any plan on committing this, if not please
> explain and close this code review.
> 
> Thanks,
> -Ivan
> 
> 
> On 2011/10/24 09:03:21, jrgfogh wrote:
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/Process...
> > File tests/standalone/src/ProcessStderrTest.dart (right):
> > 
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/Process...
> > tests/standalone/src/ProcessStderrTest.dart:36: for (int i = 0; i <
> BUFFERSIZE;
> > i++) {
> > On 2011/10/24 06:23:04, Søren Gjesse wrote:
> > > I forgot that this has to be BUFFERSIZE - 1 for the test to work on
Windows.
> > The
> > > line ending \n turns into \r\n on Windows.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/Process...
> > File tests/standalone/src/ProcessStdoutTest.dart (right):
> > 
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/Process...
> > tests/standalone/src/ProcessStdoutTest.dart:36: for (int i = 0; i <
> BUFFERSIZE;
> > i++) {
> > On 2011/10/24 06:23:04, Søren Gjesse wrote:
> > > As in ProcessStderrTest.dart
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/standalone....
> > File tests/standalone/standalone.status (right):
> > 
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/standalone....
> > tests/standalone/standalone.status:25: Process*: Skip
> > On 2011/10/21 15:42:29, Ivan Posva wrote:
> > > This whole block should go as well.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/standalone....
> > tests/standalone/standalone.status:33: Process*: Skip
> > On 2011/10/21 15:42:29, Ivan Posva wrote:
> > > ditto
> > 
> > Done.

Finally got to test this on Windows. Please make the following changes (or
something like it) to ProcessStdoutTest.dart and ProcessStderrTest.dart for the
tests to run on Windows.

diff --git a/tests/standalone/src/ProcessStdoutTest.dart
b/tests/standalone/src/ProcessStdoutTest.dart
index 0b3adb7..49208f1 100644
--- a/tests/standalone/src/ProcessStdoutTest.dart
+++ b/tests/standalone/src/ProcessStdoutTest.dart
@@ -30,20 +30,31 @@ class ProcessStdoutTest {
     process.start();

     int received = 0;
+    List<int> buffer;

     void readData() {
-      List<int> buffer = input.read();
-      for (int i = 0; i < buffer.length; i++) {
+      buffer = input.read();
+      // Check the data received. On Windows the trailing \n will become \r\n.
+      for (int i = 0; i < data.length - 1; i++) {
         Expect.equals(data[received + i], buffer[i]);
       }
       received += buffer.length;
-      if (received == BUFFERSIZE) {
-        process.close();
+      if (received >= BUFFERSIZE) {
+        if (buffer[BUFFERSIZE - 1] == 13 && received >= BUFFERSIZE + 1) {
+          process.close();
+        } else {
+          process.close();
+        }
       }
     }

     void streamClosed() {
-      Expect.equals(BUFFERSIZE, received);
+      Expect.equals(buffer[received - 1], 10);
+      if (received == BUFFERSIZE + 1) {
+        Expect.equals(buffer[received - 2], 13);
+      } else {
+        Expect.equals(BUFFERSIZE, received);
+      }
     }

     output.write(data);

Søren Gjesse

On 2011/11/03 12:37:09, Søren Gjesse wrote: > On 2011/11/02 16:42:41, Ivan Posva wrote: > > ...

9 years, 1 month ago (2011-11-03 12:38:30 UTC) #8

On 2011/11/03 12:37:09, Søren Gjesse wrote:
> On 2011/11/02 16:42:41, Ivan Posva wrote:
> > What is the status of this change? Any plan on committing this, if not
please
> > explain and close this code review.
> > 
> > Thanks,
> > -Ivan
> > 
> > 
> > On 2011/10/24 09:03:21, jrgfogh wrote:
> > >
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/Process...
> > > File tests/standalone/src/ProcessStderrTest.dart (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/Process...
> > > tests/standalone/src/ProcessStderrTest.dart:36: for (int i = 0; i <
> > BUFFERSIZE;
> > > i++) {
> > > On 2011/10/24 06:23:04, Søren Gjesse wrote:
> > > > I forgot that this has to be BUFFERSIZE - 1 for the test to work on
> Windows.
> > > The
> > > > line ending \n turns into \r\n on Windows.
> > > 
> > > Done.
> > > 
> > >
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/Process...
> > > File tests/standalone/src/ProcessStdoutTest.dart (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/src/Process...
> > > tests/standalone/src/ProcessStdoutTest.dart:36: for (int i = 0; i <
> > BUFFERSIZE;
> > > i++) {
> > > On 2011/10/24 06:23:04, Søren Gjesse wrote:
> > > > As in ProcessStderrTest.dart
> > > 
> > > Done.
> > > 
> > >
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/standalone....
> > > File tests/standalone/standalone.status (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/standalone....
> > > tests/standalone/standalone.status:25: Process*: Skip
> > > On 2011/10/21 15:42:29, Ivan Posva wrote:
> > > > This whole block should go as well.
> > > 
> > > Done.
> > > 
> > >
> >
>
http://codereview.chromium.org/8365032/diff/3002/tests/standalone/standalone....
> > > tests/standalone/standalone.status:33: Process*: Skip
> > > On 2011/10/21 15:42:29, Ivan Posva wrote:
> > > > ditto
> > > 
> > > Done.
> 
> Finally got to test this on Windows. Please make the following changes (or
> something like it) to ProcessStdoutTest.dart and ProcessStderrTest.dart for
the
> tests to run on Windows.
> 
> diff --git a/tests/standalone/src/ProcessStdoutTest.dart
> b/tests/standalone/src/ProcessStdoutTest.dart
> index 0b3adb7..49208f1 100644
> --- a/tests/standalone/src/ProcessStdoutTest.dart
> +++ b/tests/standalone/src/ProcessStdoutTest.dart
> @@ -30,20 +30,31 @@ class ProcessStdoutTest {
>      process.start();
> 
>      int received = 0;
> +    List<int> buffer;
> 
>      void readData() {
> -      List<int> buffer = input.read();
> -      for (int i = 0; i < buffer.length; i++) {
> +      buffer = input.read();
> +      // Check the data received. On Windows the trailing \n will become
\r\n.
> +      for (int i = 0; i < data.length - 1; i++) {
>          Expect.equals(data[received + i], buffer[i]);
>        }
>        received += buffer.length;
> -      if (received == BUFFERSIZE) {
> -        process.close();
> +      if (received >= BUFFERSIZE) {
> +        if (buffer[BUFFERSIZE - 1] == 13 && received >= BUFFERSIZE + 1) {
> +          process.close();
> +        } else {
> +          process.close();
> +        }
>        }
>      }
> 
>      void streamClosed() {
> -      Expect.equals(BUFFERSIZE, received);
> +      Expect.equals(buffer[received - 1], 10);
> +      if (received == BUFFERSIZE + 1) {
> +        Expect.equals(buffer[received - 2], 13);
> +      } else {
> +        Expect.equals(BUFFERSIZE, received);
> +      }
>      }
> 
>      output.write(data);

I can see my patch is not correct if data does not arrive in one chunk, but you
get the idea.

Søren Gjesse

http://codereview.chromium.org/8365032/diff/14001/tests/standalone/src/ProcessStderrTest.dart File tests/standalone/src/ProcessStderrTest.dart (right): http://codereview.chromium.org/8365032/diff/14001/tests/standalone/src/ProcessStderrTest.dart#newcode37 tests/standalone/src/ProcessStderrTest.dart:37: for (int i = received; i < Math.min(data.length, buffer.length); ...

9 years, 1 month ago (2011-11-08 08:43:38 UTC) #9

jrgfogh

9 years, 1 month ago (2011-11-08 09:15:15 UTC) #10

lgtm

Expand Messages | Collapse Messages