Make addlatexhash_test clean up its temp directories.

Make addlatexhash_test clean up its temp directories.

BUG=
R=eernst@google.com

Committed: https://code.google.com/p/dart/source/detail?r=43242

[Bill Hesse, 2015-01-27 17:18:36]

whesse@google.com changed reviewers:
+ eernst@google.com

[eernst, 2015-01-28 13:04:35]

LGTM.

I proposed a small adjustment.  To make sure we have an explicit record of the
intended semantics, I'll outline it here: The test should not leave behind any
tmp dirs, no matter whether it succeeds or fails.  On the other hand, the
failure to create a tmp dir should be made visible (as a test failure, because
we have no other signals that we can send), such that it does not mask a
potential failure to run the test itself.  We will also see failure to delete
the tmp dir as a test failure.  This may be less helpful, but hopefully it
occurs only rarely.

https://codereview.chromium.org/883793002/diff/1/tests/standalone/io/addlatex...
File tests/standalone/io/addlatexhash_test.dart (right):

https://codereview.chromium.org/883793002/diff/1/tests/standalone/io/addlatex...
tests/standalone/io/addlatexhash_test.dart:202: }
Ah, of course I should have removed those tmp files.  

But, as a small twist, maybe something like this could be used to keep main
small & readable:

runWithTmpDir(void f(String)) {
  final tmpDir = Directory.systemTemp.createTempSync("addlatexhash_test");
  try {
    f(tmpDir.path);
  }
  finally {
    tmpDir.delete(recursive: true);
  }
}

and then in main:

  runWithTmpDir(testSameHash);
  ..
  if (..) runWithTestDir(testSameDVI);

[Bill Hesse, 2015-01-28 13:31:04]

It is not clear what you mean by this.  This description of the semantics sounds
like
what the current proposed change does - errors in the directory creation or
deletion
will cause a test failure.

Is your proposed small adjustment the refactoring into runWithTmpDir?
This should not change the failure semantics.


On 2015/01/28 13:04:35, eernst wrote:
> LGTM.
> 
> I proposed a small adjustment.  To make sure we have an explicit record of the
> intended semantics, I'll outline it here: The test should not leave behind any
> tmp dirs, no matter whether it succeeds or fails.  On the other hand, the
> failure to create a tmp dir should be made visible (as a test failure, because
> we have no other signals that we can send), such that it does not mask a
> potential failure to run the test itself.  We will also see failure to delete
> the tmp dir as a test failure.  This may be less helpful, but hopefully it
> occurs only rarely.
> 
>
https://codereview.chromium.org/883793002/diff/1/tests/standalone/io/addlatex...
> File tests/standalone/io/addlatexhash_test.dart (right):
> 
>
https://codereview.chromium.org/883793002/diff/1/tests/standalone/io/addlatex...
> tests/standalone/io/addlatexhash_test.dart:202: }
> Ah, of course I should have removed those tmp files.  
> 
> But, as a small twist, maybe something like this could be used to keep main
> small & readable:
> 
> runWithTmpDir(void f(String)) {
>   final tmpDir = Directory.systemTemp.createTempSync("addlatexhash_test");
>   try {
>     f(tmpDir.path);
>   }
>   finally {
>     tmpDir.delete(recursive: true);
>   }
> }
> 
> and then in main:
> 
>   runWithTmpDir(testSameHash);
>   ..
>   if (..) runWithTestDir(testSameDVI);

[Bill Hesse, 2015-01-28 13:41:34]

https://codereview.chromium.org/883793002/diff/20001/tests/standalone/io/addl...
File tests/standalone/io/addlatexhash_test.dart (right):

https://codereview.chromium.org/883793002/diff/20001/tests/standalone/io/addl...
tests/standalone/io/addlatexhash_test.dart:184: runWithTempDir(void test(String
tempDir)) {
Changed name of function to runWithTempDir.  It is a standard in the Dart
project (and across Google) that we always write words fully spelled out, never
abbreviating. It ensures that there is never a question about how you abbreviate
a particular function name.

[Bill Hesse, 2015-01-28 13:41:52]

Committed patchset #2 (id:20001) manually as 43242 (presubmit successful).

[eernst, 2015-01-28 13:46:21]

On 2015/01/28 13:31:04, Bill Hesse wrote:
> It is not clear what you mean by this.  This description of the semantics
sounds
> like
> what the current proposed change does - errors in the directory creation or
> deletion
> will cause a test failure.

Exactly, I was just retracing the semantics explicitly and concluding that it
would be as intended (after having considered other things like "shouldn't we
protect the invocation of createTempSync and delete as well?").  I put it in the
message just so that you could take a look and (hopefully ;) reach the same
conclusion.

> Is your proposed small adjustment the refactoring into runWithTmpDir?

Yes, that's all.

> This should not change the failure semantics.

Agreed.

> 
> On 2015/01/28 13:04:35, eernst wrote:
> > LGTM.
> > 
> > I proposed a small adjustment.  To make sure we have an explicit record of
the
> > intended semantics, I'll outline it here: The test should not leave behind
any
> > tmp dirs, no matter whether it succeeds or fails.  On the other hand, the
> > failure to create a tmp dir should be made visible (as a test failure,
because
> > we have no other signals that we can send), such that it does not mask a
> > potential failure to run the test itself.  We will also see failure to
delete
> > the tmp dir as a test failure.  This may be less helpful, but hopefully it
> > occurs only rarely.
> > 
> >
>
https://codereview.chromium.org/883793002/diff/1/tests/standalone/io/addlatex...
> > File tests/standalone/io/addlatexhash_test.dart (right):
> > 
> >
>
https://codereview.chromium.org/883793002/diff/1/tests/standalone/io/addlatex...
> > tests/standalone/io/addlatexhash_test.dart:202: }
> > Ah, of course I should have removed those tmp files.  
> > 
> > But, as a small twist, maybe something like this could be used to keep main
> > small & readable:
> > 
> > runWithTmpDir(void f(String)) {
> >   final tmpDir = Directory.systemTemp.createTempSync("addlatexhash_test");
> >   try {
> >     f(tmpDir.path);
> >   }
> >   finally {
> >     tmpDir.delete(recursive: true);
> >   }
> > }
> > 
> > and then in main:
> > 
> >   runWithTmpDir(testSameHash);
> >   ..
> >   if (..) runWithTestDir(testSameDVI);

[eernst, 2015-01-28 13:49:27]

lgtm