[GitHub] [arrow] sbinet commented on a change in pull request #7823: ARROW-9548: [Go] Test output files are not removed correctly

[GitBox <gi...@apache.org>]

sbinet commented on a change in pull request #7823:
URL: https://github.com/apache/arrow/pull/7823#discussion_r465023873



##########
File path: go/arrow/ipc/cmd/arrow-json-integration-test/main.go
##########
@@ -158,16 +148,6 @@ func cnvToARROW(arrowName, jsonName string, verbose bool) error {
 		return xerrors.Errorf("invalid number of records copied (got=%d, want=%d", got, want)
 	}
 
-	err = ww.Close()

Review comment:
       ditto

##########
File path: go/arrow/ipc/cmd/arrow-json-integration-test/main_test.go
##########
@@ -39,8 +39,8 @@ func TestIntegration(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
+			defer os.Remove(af1.Name())
 			defer af1.Close()
-			defer os.RemoveAll(af1.Name())

Review comment:
       same comment than above: perhaps replace with a per-sub-test top-level tmp dir that is being `defer os.RemoveAll(tmpdir)` ?

##########
File path: go/arrow/arrio/arrio_test.go
##########
@@ -98,15 +98,15 @@ func TestCopy(t *testing.T) {
 							if err != nil {
 								t.Fatal(err)
 							}
-							defer f.Close()
 							defer os.Remove(f.Name())
+							defer f.Close()

Review comment:
       hum... perhaps we should just create a per-sub-test tmp directory and `defer os.RemoveAll(tmpdir)` instead?
   (and loose the `defer os.Remove(file.Name())` calls)

##########
File path: go/arrow/ipc/cmd/arrow-json-integration-test/main.go
##########
@@ -108,16 +108,6 @@ func cnvToJSON(arrowName, jsonName string, verbose bool) error {
 		return xerrors.Errorf("invalid number of records copied (got=%d, want=%d", got, want)
 	}
 
-	err = ww.Close()

Review comment:
       I'd prefer to leave those untouched, so the failure, if any, is visible.
   the `defer ww.Close()` from line 100 won't tell the user something bad happened (and the double close will be silent.)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org