code review 7313062: exp/ssa/interp: (#6 of 5): test interpretation of SSA f...

exp/ssa/interp: (#6 of 5): test interpretation of SSA form of $GOROOT/test/*.go.

The interpreter's os.Exit now triggers a special panic rather
than kill the test process.  (It's semantically dubious, since
it will run deferred routines.)  Interpret now returns its
exit code rather than calling os.Exit.

Also:
- disabled parts of a few $GOROOT/tests via os.Getenv("GOSSAINTERP").
- remove unnecessary 'slots' param to external functions; they
  are never closures.

Most of the tests are disabled until go/types supports shifts.
They can be reenabled if you patch this workaround:
https://codereview.appspot.com/7312068

[adonovan, 2013-02-08 22:00:50]

Hello iant@golang.org (cc: golang-dev@googlegroups.com, gri@golang.org),

I'd like you to review this change to
https://code.google.com/p/go/

[iant, 2013-02-08 23:10:23]

https://codereview.appspot.com/7313062/diff/4/test/blank.go
File test/blank.go (right):

https://codereview.appspot.com/7313062/diff/4/test/blank.go#newcode112
test/blank.go:112: if os.Getenv("GOSSAINTERP") == "" {
I only looked at this test, but let's not do this.  One option: don't do it at
all, in the assumption that you will be able to fix these.  Another: the tests
all have an indicator at the top that indicates how to execute the test; we can
extend as appropriate to say that the test should not be run when using the
interpreter.

[adonovan, 2013-02-08 23:21:50]

https://codereview.appspot.com/7313062/diff/4/test/blank.go
File test/blank.go (right):

https://codereview.appspot.com/7313062/diff/4/test/blank.go#newcode112
test/blank.go:112: if os.Getenv("GOSSAINTERP") == "" {
On 2013/02/08 23:10:23, iant wrote:
> I only looked at this test, but let's not do this.  One option: don't do it at
> all, in the assumption that you will be able to fix these.  Another: the tests
> all have an indicator at the top that indicates how to execute the test; we
can
> extend as appropriate to say that the test should not be run when using the
> interpreter.

I thought this was a reasonable compromise that allowed us to run the majority
of $GOROOT/tests without modification.  There are only 5 occurrences of this
check of which 3 will disappear in due course (and 2 likely never).  The test
programs containing the 3 provide a great deal of coverage we don't get from
elsewhere, hence my desire not to wait until the interpreter is perfect before
including these tests.

Russ thought this was a reasonable approach, FWIW.

[iant, 2013-02-11 16:56:54]

Sorry about this, but I don't think most of the changes to the test directory
should be bundled together with interpreter changes.  Many of the test changes
are straight cleanups--you can just put those in a single CL grouped together. 
A couple of the test changes add additional tests--those should be in their own
separate CLs with just those additional tests.

That leaves this CL with the interpreter changes and a few test changes to test
the environment variable.  I'm still not really happy with the environment
variable but I guess we can do it if we must.

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp.go
File src/pkg/exp/ssa/interp/interp.go (right):

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp.g...
src/pkg/exp/ssa/interp/interp.go:597: exitCode = 1
Looks to me like this will cause a panic: unexpected type: nil.

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp_t...
File src/pkg/exp/ssa/interp/interp_test.go (right):

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp_t...
src/pkg/exp/ssa/interp/interp_test.go:14: red   = "\x1b[1;31m"
This test is going to run as part of ordinary runs of make.bash.  It shouldn't
print ANSI terminal sequences, it should just call t.Error and so on.  You can
add an option to print those sequences if you want for use with this specific
package.

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp_t...
src/pkg/exp/ssa/interp/interp_test.go:24: // the $GOROOT/test driver scripts,
golden file checking, etc?
I would think it would make sense at some point.

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp_t...
src/pkg/exp/ssa/interp/interp_test.go:134: fmt.Printf("Input: %s\n", input)
Tests should normally use t.Log, not fmt.Print.  fmt.Print is not forbidden but
why not use t.Log?

https://codereview.appspot.com/7313062/diff/4/test/method.go
File test/method.go (right):

https://codereview.appspot.com/7313062/diff/4/test/method.go#newcode254
test/method.go:254: type A struct{ B }
This change doesn't seem related to the interpreter per se; can we split it out
into a different CL?

https://codereview.appspot.com/7313062/diff/4/test/range.go
File test/range.go (right):

https://codereview.appspot.com/7313062/diff/4/test/range.go#newcode169
test/range.go:169: // Non-evaluation of the range expression is permitted for
arrays.
We have to be clear about whether the range expression is evaluated or not. 
There shouldn't be any "is permitted" for something like that.  Please break
this out into a separate CL, and let's test for precisely the number of
executions.

https://codereview.appspot.com/7313062/diff/4/test/reorder.go
File test/reorder.go (right):

https://codereview.appspot.com/7313062/diff/4/test/reorder.go#newcode109
test/reorder.go:109: // See http://code.google.com/p/go/issues/detail?id=4627.
This comment makes sense as part of this CL but it does not make sense in the
context of the test.  I think it can simply be omitted.

https://codereview.appspot.com/7313062/diff/4/test/switch.go
File test/switch.go (right):

https://codereview.appspot.com/7313062/diff/4/test/switch.go#newcode331
test/switch.go:331: switch {
Again, additional testing should be a separate CL.

[adonovan, 2013-02-11 20:59:19]

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp.go
File src/pkg/exp/ssa/interp/interp.go (right):

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp.g...
src/pkg/exp/ssa/interp/interp.go:597: exitCode = 1
On 2013/02/11 16:56:55, iant wrote:
> Looks to me like this will cause a panic: unexpected type: nil.

Good catch.  I've changed the condition dominating the deferred func to exitCode
!= 2.

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp_t...
File src/pkg/exp/ssa/interp/interp_test.go (right):

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp_t...
src/pkg/exp/ssa/interp/interp_test.go:14: red   = "\x1b[1;31m"
On 2013/02/11 16:56:55, iant wrote:
> This test is going to run as part of ordinary runs of make.bash.  It shouldn't
> print ANSI terminal sequences, it should just call t.Error and so on.  You can
> add an option to print those sequences if you want for use with this specific
> package.

I've put this behind a -color flag.

FWIW, the color is not frivolous; it makes it immediately obvious when viewing a
log of a test in progress whether it has failed.  The words PASS and FAIL by
themselves don't leap off the page.

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp_t...
src/pkg/exp/ssa/interp/interp_test.go:24: // the $GOROOT/test driver scripts,
golden file checking, etc?
On 2013/02/11 16:56:55, iant wrote:
> I would think it would make sense at some point.

Ok; reworded the TODO.

https://codereview.appspot.com/7313062/diff/4/src/pkg/exp/ssa/interp/interp_t...
src/pkg/exp/ssa/interp/interp_test.go:134: fmt.Printf("Input: %s\n", input)
On 2013/02/11 16:56:55, iant wrote:
> Tests should normally use t.Log, not fmt.Print.  fmt.Print is not forbidden
but
> why not use t.Log?

The TestInterp function runs dozens of tests; t.Log isn't printed until after
the last one (and not at all on success, of course).  During development I have
found it a great saving of time to get immediate feedback about failing tests by
printing to stdout like testing.T itself does when it heralds each new test
function.

https://codereview.appspot.com/7313062/diff/4/test/method.go
File test/method.go (right):

https://codereview.appspot.com/7313062/diff/4/test/method.go#newcode254
test/method.go:254: type A struct{ B }
On 2013/02/11 16:56:55, iant wrote:
> This change doesn't seem related to the interpreter per se; can we split it
out
> into a different CL?

Moved to https://codereview.appspot.com/7306087.

https://codereview.appspot.com/7313062/diff/4/test/range.go
File test/range.go (right):

https://codereview.appspot.com/7313062/diff/4/test/range.go#newcode169
test/range.go:169: // Non-evaluation of the range expression is permitted for
arrays.
On 2013/02/11 16:56:55, iant wrote:
> We have to be clear about whether the range expression is evaluated or not. 
> There shouldn't be any "is permitted" for something like that.  Please break
> this out into a separate CL, and let's test for precisely the number of
> executions.

Changes to the spec wording are being discussed in CL 
https://codereview.appspot.com/7307083/, and once the dust has settled that CL
ought to add assertions to this test.  (Changes to the SSA builder may be
necessary too.)

I've moved the (non-semantic) println changes to
https://codereview.appspot.com/7306087.

https://codereview.appspot.com/7313062/diff/4/test/reorder.go
File test/reorder.go (right):

https://codereview.appspot.com/7313062/diff/4/test/reorder.go#newcode109
test/reorder.go:109: // See http://code.google.com/p/go/issues/detail?id=4627.
On 2013/02/11 16:56:55, iant wrote:
> This comment makes sense as part of this CL but it does not make sense in the
> context of the test.  I think it can simply be omitted.

Done.

https://codereview.appspot.com/7313062/diff/4/test/switch.go
File test/switch.go (right):

https://codereview.appspot.com/7313062/diff/4/test/switch.go#newcode331
test/switch.go:331: switch {
On 2013/02/11 16:56:55, iant wrote:
> Again, additional testing should be a separate CL.

Moved to https://codereview.appspot.com/7306087.

[iant, 2013-02-12 00:40:49]

On 2013/02/11 20:59:19, adonovan wrote:

> src/pkg/exp/ssa/interp/interp_test.go:134: fmt.Printf("Input: %s\n", input)
> On 2013/02/11 16:56:55, iant wrote:
> > Tests should normally use t.Log, not fmt.Print.  fmt.Print is not forbidden
> but
> > why not use t.Log?
> 
> The TestInterp function runs dozens of tests; t.Log isn't printed until after
> the last one (and not at all on success, of course).  During development I
have
> found it a great saving of time to get immediate feedback about failing tests
by
> printing to stdout like testing.T itself does when it heralds each new test
> function.

I suppose the important thing is that "bash all.bash" should not dump any output
when testing the directory.

[iant, 2013-02-12 00:40:58]

https://codereview.appspot.com/7313062/diff/27/test/cmplxdivide.go
File test/cmplxdivide.go (right):

https://codereview.appspot.com/7313062/diff/27/test/cmplxdivide.go#newcode49
test/cmplxdivide.go:49: panic("cmplxdivide failed.")
I thought these simple cleanups were moving to a different CL?

[adonovan, 2013-02-12 03:23:16]

https://codereview.appspot.com/7313062/diff/27/test/cmplxdivide.go
File test/cmplxdivide.go (right):

https://codereview.appspot.com/7313062/diff/27/test/cmplxdivide.go#newcode49
test/cmplxdivide.go:49: panic("cmplxdivide failed.")
On 2013/02/12 00:40:58, iant wrote:
> I thought these simple cleanups were moving to a different CL?

These simple cleanups are conceptually part of this change.
Though they are not required for correctness of the tests run by make.bash,
which checks the output against a golden file, they are required by
exp/ssa/interp/interp_test.go which expects tests to exit 0 iff they are
successful, thus all failing tests must exit nonzero or panic in addition to
perhaps printing some error messages.

This isn't ideal since it means these tests must be written to conform to two
slightly different sets of expectations, but it seems like a pragmatic
compromise for the medium term.   Doing the "right" thing (integrating
exp/ssa/interp into the scripts that drive the existing tests) would be a fair
amount of work for diminishing returns, I think.

[iant, 2013-02-12 05:53:14]

We changed away from a golden file a while back.  The tests are now run by
test/run.go.  A test fails by exiting with a non-zero status.  If a test exits
with a zero status, it is counted as passing.  So in fact the changes you are
making to the tests, other than checking the environment variable, are
corrections.  Those tests are broken, in the sense that they will not be
correctly detected as failing if they do fail.  We don't notice because 1) they
don't fail; 2) if they do fail, run.go won't pick it up.

Hence I think the changes to the tests other than the environment variable
should be in a different CL.

[bradfitz, 2013-02-12 06:09:45]

On Mon, Feb 11, 2013 at 9:53 PM, <iant@golang.org> wrote:

> We changed away from a golden file a while back.  The tests are now run
> by test/run.go.


They're now _also_ ran by run.go.  But run.go and run (sh) are kept in sync.


>  A test fails by exiting with a non-zero status.  If a
> test exits with a zero status, it is counted as passing.


Unless the test type is "cmpout", or is one of the "error*" types.

[adonovan, 2013-02-12 16:26:05]

Thanks for the clarifications; in that case I will make another CL to
ensure that all tests exit nonzero on failure.




On 12 February 2013 01:09, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> On Mon, Feb 11, 2013 at 9:53 PM, <iant@golang.org> wrote:
>
>> We changed away from a golden file a while back.  The tests are now run
>> by test/run.go.
>
>
> They're now _also_ ran by run.go.  But run.go and run (sh) are kept in
> sync.
>
>
>>  A test fails by exiting with a non-zero status.  If a
>> test exits with a zero status, it is counted as passing.
>
>
> Unless the test type is "cmpout", or is one of the "error*" types.
>
>
>

[adonovan, 2013-02-13 05:43:03]

(PTAL)

On 2013/02/12 16:26:05, adonovan wrote:
> Thanks for the clarifications; in that case I will make another CL to
> ensure that all tests exit nonzero on failure.
> 
> 
> 
> 
> On 12 February 2013 01:09, Brad Fitzpatrick <mailto:bradfitz@golang.org>
wrote:
> 
> > On Mon, Feb 11, 2013 at 9:53 PM, <mailto:iant@golang.org> wrote:
> >
> >> We changed away from a golden file a while back.  The tests are now run
> >> by test/run.go.
> >
> >
> > They're now _also_ ran by run.go.  But run.go and run (sh) are kept in
> > sync.
> >
> >
> >>  A test fails by exiting with a non-zero status.  If a
> >> test exits with a zero status, it is counted as passing.
> >
> >
> > Unless the test type is "cmpout", or is one of the "error*" types.
> >
> >
> >

[iant, 2013-02-13 06:03:20]

LGTM

[adonovan, 2013-02-21 17:48:41]

*** Submitted as https://code.google.com/p/go/source/detail?r=577ba2d07b0b ***

exp/ssa/interp: (#6 of 5): test interpretation of SSA form of $GOROOT/test/*.go.

The interpreter's os.Exit now triggers a special panic rather
than kill the test process.  (It's semantically dubious, since
it will run deferred routines.)  Interpret now returns its
exit code rather than calling os.Exit.

Also:
- disabled parts of a few $GOROOT/tests via os.Getenv("GOSSAINTERP").
- remove unnecessary 'slots' param to external functions; they
  are never closures.

Most of the tests are disabled until go/types supports shifts.
They can be reenabled if you patch this workaround:
https://codereview.appspot.com/7312068

R=iant, bradfitz
CC=golang-dev, gri
https://codereview.appspot.com/7313062