Add generated function_type tests.

Add generated function_type tests.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/1c355af468c368af9945ef747f02b874654493d4

[floitsch, 2017-02-22 16:04:17]

floitsch@google.com changed reviewers:
+ ahe@google.com

[floitsch, 2017-02-22 16:04:18]

Add the tests that were excluded in https://codereview.chromium.org/2656743002

[floitsch, 2017-03-22 23:34:15]

ping.

Also the CL was https://codereview.chromium.org/2567133002

[ahe, 2017-03-23 11:31:38]

This is a big CL, and I'm not sure how to begin reviewing it.

[floitsch, 2017-03-23 17:48:03]

On 2017/03/23 11:31:38, ahe wrote:
> This is a big CL, and I'm not sure how to begin reviewing it.

These are all generated tests. The generator (and the tests themselves) were
already reviewed in https://codereview.chromium.org/2567133002 by Siggi.
In that CL you asked me to move the tests into a separate CL (this one).

If you have doubts on the tests themselves I would have a look at the (already
committed) generator.

[ahe, 2017-03-24 08:35:53]

On 2017/03/23 17:48:03, floitsch wrote:
> On 2017/03/23 11:31:38, ahe wrote:
> > This is a big CL, and I'm not sure how to begin reviewing it.
> 
> These are all generated tests. The generator (and the tests themselves) were
> already reviewed in https://codereview.chromium.org/2567133002 by Siggi.
> In that CL you asked me to move the tests into a separate CL (this one).
> 
> If you have doubts on the tests themselves I would have a look at the (already
> committed) generator.

If Siggi already reviewed the generator, I would feel more comfortable if he
could also review this CL.

[floitsch, 2017-03-24 14:00:34]

floitsch@google.com changed reviewers:
+ sigmund@google.com
- ahe@google.com

[Siggi Cherem (dart-lang), 2017-03-24 15:51:38]

I'm glad we are splitting these in multiple files, thank you - the humongous
file we had before was just too hard to work with.

lgtm - I have a few style suggestions for improving the generator to make these
generated files easier to follow, but I'm OK if you want to land this and do
those as a follow up.

My suggestions:
 - improving the naming scheme: now that it's split, seeing a type like F2304
feels like a lot of noise. Some ideas:
     - use just a counter local to the file (typeCounter / units.length)
     - maybe device a scheme to produce a meaninful name from the type you are
testing?

 - improving the clustering scheme: right now you are putting all tests together
in a bag, then sharding them based on where they landed on a list. Maybe we can
add them more strategically (e.g. group1: only basic argument types, ...
group-n: all types using optional named args, etc)?

 - more docs:
   - Consider adding a library tag with a dartdoc explaining the kind of tests
each file has. I'd include 2 things:
     - a general description of what's common of all test files (simple function
types, nested function types, function types returning primitives or lists with
a type-arg, etc)
     - if we improve clustering, then you can include what kind of cluster each
file has (e.g. this file covers function-types with named arguments, this file
covers function types with generics, ...)

   - moving some comments in the generated code to be dartdocs, and include some
text as well if possible (I included an example in a comment on the review
below)


Cheers!

https://codereview.chromium.org/2707033004/diff/1/tests/language/function_typ...
File tests/language/function_type/function_type93_test.dart (right):

https://codereview.chromium.org/2707033004/diff/1/tests/language/function_typ...
tests/language/function_type/function_type93_test.dart:211: // Function(int x0,
[Function x])
Could we move this first comment out as a dartdoc and explain it in words too:

/// Checks function types of the form:
///
///     Function(int x0, [Function x])
///
/// that include a required argument and an optional Function argument.

[floitsch, 2017-04-19 13:30:18]

On 2017/03/24 15:51:38, Siggi Cherem (dart-lang) wrote:
> I'm glad we are splitting these in multiple files, thank you - the humongous
> file we had before was just too hard to work with.
> 
> lgtm - I have a few style suggestions for improving the generator to make
these
> generated files easier to follow, but I'm OK if you want to land this and do
> those as a follow up.
> 
> My suggestions:
>  - improving the naming scheme: now that it's split, seeing a type like F2304
> feels like a lot of noise. Some ideas:
>      - use just a counter local to the file (typeCounter / units.length)
done.

>      - maybe device a scheme to produce a meaninful name from the type you are
> testing?
I don't see what that would buy us and tbh I wouldn't know how.

> 
>  - improving the clustering scheme: right now you are putting all tests
together
> in a bag, then sharding them based on where they landed on a list. Maybe we
can
> add them more strategically (e.g. group1: only basic argument types, ...
> group-n: all types using optional named args, etc)?
I much rather have all of them mixed up, so that running one tests pretty much
covers everything. The remaining tests are then mostly redundant.

> 
>  - more docs:
>    - Consider adding a library tag with a dartdoc explaining the kind of tests
> each file has. I'd include 2 things:
>      - a general description of what's common of all test files (simple
function
> types, nested function types, function types returning primitives or lists
with
> a type-arg, etc)
>      - if we improve clustering, then you can include what kind of cluster
each
> file has (e.g. this file covers function-types with named arguments, this file
> covers function types with generics, ...)
> 
>    - moving some comments in the generated code to be dartdocs, and include
some
> text as well if possible (I included an example in a comment on the review
> below)
Moved the type as a dartdoc.
> 
> 
> Cheers!
> 
>
https://codereview.chromium.org/2707033004/diff/1/tests/language/function_typ...
> File tests/language/function_type/function_type93_test.dart (right):
> 
>
https://codereview.chromium.org/2707033004/diff/1/tests/language/function_typ...
> tests/language/function_type/function_type93_test.dart:211: // Function(int
x0,
> [Function x])
> Could we move this first comment out as a dartdoc and explain it in words too:
> 
> /// Checks function types of the form:
> ///
> ///     Function(int x0, [Function x])
> ///
> /// that include a required argument and an optional Function argument.

[floitsch, 2017-04-19 13:30:37]

https://codereview.chromium.org/2707033004/diff/1/tests/language/function_typ...
File tests/language/function_type/function_type93_test.dart (right):

https://codereview.chromium.org/2707033004/diff/1/tests/language/function_typ...
tests/language/function_type/function_type93_test.dart:211: // Function(int x0,
[Function x])
On 2017/03/24 15:51:38, Siggi Cherem (dart-lang) wrote:
> Could we move this first comment out as a dartdoc and explain it in words too:
> 
> /// Checks function types of the form:
> ///
> ///     Function(int x0, [Function x])
> ///
> /// that include a required argument and an optional Function argument.

Moved it to as a dartdoc.

Didn't add "Checks function types of the form"..., since that would be just
repeated hundreds of time and would be the same everywhere.

Didn't create human readable text. The function type is not that hard to parse.

[floitsch, 2017-04-19 13:56:54]

Description was changed from

==========
Add generated function_type tests.
==========

to

==========
Add generated function_type tests.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1c355af468c368af9945ef747f02b874654493d4
==========

[floitsch, 2017-04-19 13:56:56]

Committed patchset #3 (id:20001) manually as
1c355af468c368af9945ef747f02b874654493d4.