Add initial tests for Kernel interpreter

Add initial tests for Kernel interpreter

Add logging in the Interpreter for:
 - StaticInvocationExpression, including calls to print
 - ReturnStatement
 - IfStatement


BUG=
R=dmitryas@google.com

Committed: https://github.com/dart-lang/sdk/commit/184cc2576685ba74417ec44d547ea5172394c447

[zhivkag, 2017-04-28 13:09:13]

zhivkag@google.com changed reviewers:
+ dmitryas@google.com, kmillikin@google.com

[zhivkag, 2017-04-28 13:09:14]

Hi, 

This change adds initial tests for the interpreter.
It adds logging when evaluating different expressions and/or executing
statements. After successful run of a program by the interpreter, the log is
compared with the expected output. 
Currently, we log static invocation (including print) expressions and return
statements. The added tests are not meant to be exhaustive.
For generating the Kernel AST of a program, I used the same step as in
test/closures/suite.dart  

Any comments or suggestions are much appreciated! 

Thanks, 
Zhivka

[Dmitry Stefantsov, 2017-05-01 09:35:24]

I think these tests are really useful to have! I have a couple of comments,
though. Please, find them below.

Additionally, it would be nice to add the list of configurations that are
currently logged to the description of the CL, and also add small descriptions
with the purpose of the tests in the comments of the test files.

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/test/interpr...
File pkg/kernel/test/interpreter/suite.dart (right):

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/test/interpr...
pkg/kernel/test/interpreter/suite.dart:121: class Interpret extends
Step<Program, Program, TestContext> {
I think it's better to split Interpret into two steps: Interpret and a matching
step that looks like MatchExpectation. It'll probably be a bit easier to locate
an error.

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
File pkg/kernel/testcases/interpreter/print_test.dart (right):

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
pkg/kernel/testcases/interpreter/print_test.dart:1: main() {
I think this file needs a copyright statement on top, just like any other test
file. Same for the other test file in this CL.

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
File pkg/kernel/testcases/interpreter/static_invocation_test.dart.expect
(right):

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
pkg/kernel/testcases/interpreter/static_invocation_test.dart.expect:16: return
Shouldn't this return be a couple of lines further?

[zhivkag, 2017-05-01 11:26:49]

Description was changed from

==========
Add initial tests for Kernel interpreter


BUG=
==========

to

==========
Add initial tests for Kernel interpreter

Add logging in the Interpreter for: 
 - StaticInvocationExpression, including calls to print
 - ReturnStatement
 - IfStatement


BUG=
==========

[zhivkag, 2017-05-01 11:36:20]

Thanks for the comments, Dima!
I have added an additional step for matching the output log and some comments,
what do you think?

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/test/interpr...
File pkg/kernel/test/interpreter/suite.dart (right):

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/test/interpr...
pkg/kernel/test/interpreter/suite.dart:121: class Interpret extends
Step<Program, Program, TestContext> {
On 2017/05/01 09:35:23, Dmitry Stefantsov wrote:
> I think it's better to split Interpret into two steps: Interpret and a
matching
> step that looks like MatchExpectation. It'll probably be a bit easier to
locate
> an error.

Good idea, thanks!

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
File pkg/kernel/testcases/interpreter/print_test.dart (right):

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
pkg/kernel/testcases/interpreter/print_test.dart:1: main() {
On 2017/05/01 09:35:24, Dmitry Stefantsov wrote:
> I think this file needs a copyright statement on top, just like any other test
> file. Same for the other test file in this CL.

Done.

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
File pkg/kernel/testcases/interpreter/static_invocation_test.dart.expect
(right):

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
pkg/kernel/testcases/interpreter/static_invocation_test.dart.expect:16: return
On 2017/05/01 09:35:24, Dmitry Stefantsov wrote:
> Shouldn't this return be a couple of lines further?

Return appears here because we log it when an ReturnStatement is executed. For
example the snippet: 
foo() {
 return print(0);
}
will have log that corresponds to:
return     // log when executing return statement
print(0)   // log for static invocation of print as result of the evaluation of
the return expression in the ReturnStatement.

[Dmitry Stefantsov, 2017-05-01 13:29:49]

LGTM with a small comment below.

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
File pkg/kernel/testcases/interpreter/static_invocation_test.dart.expect
(right):

https://codereview.chromium.org/2841803002/diff/20001/pkg/kernel/testcases/in...
pkg/kernel/testcases/interpreter/static_invocation_test.dart.expect:16: return
On 2017/05/01 11:36:20, zhivkag wrote:
> On 2017/05/01 09:35:24, Dmitry Stefantsov wrote:
> > Shouldn't this return be a couple of lines further?
> 
> Return appears here because we log it when an ReturnStatement is executed. For
> example the snippet: 
> foo() {
>  return print(0);
> }
> will have log that corresponds to:
> return     // log when executing return statement
> print(0)   // log for static invocation of print as result of the evaluation
of
> the return expression in the ReturnStatement.

Ah! Now I got it. Thanks!

https://codereview.chromium.org/2841803002/diff/40001/pkg/kernel/test/interpr...
File pkg/kernel/test/interpreter/suite.dart (right):

https://codereview.chromium.org/2841803002/diff/40001/pkg/kernel/test/interpr...
pkg/kernel/test/interpreter/suite.dart:135: new Interpreter(program).run();
I think we may check here if .run() throws an exception, and return a "crash"
rather than a "pass" in this case. I guess in future, when we'll interpret
throws, we'll need to come up with a better way to distinguish between
exceptions in the interpreter and exceptions in the programs being interpreted.

[zhivkag, 2017-05-01 14:11:48]

Thanks!

https://codereview.chromium.org/2841803002/diff/40001/pkg/kernel/test/interpr...
File pkg/kernel/test/interpreter/suite.dart (right):

https://codereview.chromium.org/2841803002/diff/40001/pkg/kernel/test/interpr...
pkg/kernel/test/interpreter/suite.dart:135: new Interpreter(program).run();
On 2017/05/01 13:29:49, Dmitry Stefantsov wrote:
> I think we may check here if .run() throws an exception, and return a "crash"
> rather than a "pass" in this case. I guess in future, when we'll interpret
> throws, we'll need to come up with a better way to distinguish between
> exceptions in the interpreter and exceptions in the programs being
interpreted.

It makes more sense, thanks!

[zhivkag, 2017-05-01 14:12:41]

Description was changed from

==========
Add initial tests for Kernel interpreter

Add logging in the Interpreter for: 
 - StaticInvocationExpression, including calls to print
 - ReturnStatement
 - IfStatement


BUG=
==========

to

==========
Add initial tests for Kernel interpreter

Add logging in the Interpreter for:
 - StaticInvocationExpression, including calls to print
 - ReturnStatement
 - IfStatement


BUG=
R=dmitryas@google.com

Committed:
https://github.com/dart-lang/sdk/commit/184cc2576685ba74417ec44d547ea5172394c447
==========

[zhivkag, 2017-05-01 14:12:42]

Committed patchset #4 (id:60001) manually as
184cc2576685ba74417ec44d547ea5172394c447 (presubmit successful).