Single unit test for a single library.

Single unit test for a single library.

R=efortuna@google.com

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

[Andrei Mouravski, 2013-06-27 18:02:04]

https://codereview.chromium.org/18089006/diff/1005/pkg/docgen/test/single_lib...
File pkg/docgen/test/single_library_test.dart (right):

https://codereview.chromium.org/18089006/diff/1005/pkg/docgen/test/single_lib...
pkg/docgen/test/single_library_test.dart:1: import 'dart:io';
There should be a library for this file.

https://codereview.chromium.org/18089006/diff/1005/pkg/docgen/test/single_lib...
pkg/docgen/test/single_library_test.dart:2: import
'package:unittest/unittest.dart';
One line between import blocks.

https://codereview.chromium.org/18089006/diff/1005/pkg/docgen/test/single_lib...
pkg/docgen/test/single_library_test.dart:10: setUp(() {
Newline before setUp().

https://codereview.chromium.org/18089006/diff/1005/pkg/docgen/test/single_lib...
pkg/docgen/test/single_library_test.dart:15: * Doc comment for class A.
You're missing a space before the * here and below.

https://codereview.chromium.org/18089006/diff/1005/pkg/docgen/test/single_lib...
pkg/docgen/test/single_library_test.dart:57: test('Test library name', () {
These are not different tests, they're all testing the same thing.

Everything you have now in setup, teardown, and each of these tests should be
part of one test.

However, this is no longer a unittest, it's an integration test.

Talk to Tate, since I had the same discussion with him, then have HIM review
your CL.

[Tate Mandel, 2013-06-28 16:43:29]

https://codereview.chromium.org/18089006/diff/2003/pkg/docgen/test/single_lib...
File pkg/docgen/test/single_library_test.dart (right):

https://codereview.chromium.org/18089006/diff/2003/pkg/docgen/test/single_lib...
pkg/docgen/test/single_library_test.dart:17: file.writeAsStringSync('''
You can probably just include this file in the test directory instead of
creating it and deleting it every time.

https://codereview.chromium.org/18089006/diff/2003/pkg/docgen/test/single_lib...
pkg/docgen/test/single_library_test.dart:62: test('Test library generated is of
type Library', () {
All of your tests are testing the same unit of code. They should all be combined
into a single test with several expect() calls. The setUp and tearDown should
also be in this single test since they are specific to this test and wouldn't
work if you were to add other tests for different example libraries.

https://codereview.chromium.org/18089006/diff/2003/pkg/docgen/test/single_lib...
pkg/docgen/test/single_library_test.dart:77:
expect(library.classes.values.first.methods['doThis'].parameters.values
If you combine all of these tests into a single block, then you can save local
variables at each step to avoid all of the 'library._._._...'

[janicejl, 2013-06-28 23:03:06]

I have completely rewritten the unit test to hopefully be more like a unit test.
I got feedback from Tate multiple times throughout that process.

[Andrei Mouravski, 2013-07-01 19:54:29]

https://codereview.chromium.org/18089006/diff/17001/pkg/docgen/test/single_li...
File pkg/docgen/test/single_library_test.dart (right):

https://codereview.chromium.org/18089006/diff/17001/pkg/docgen/test/single_li...
pkg/docgen/test/single_library_test.dart:11: test('Single file test.', () {
I'd recommend having a top level group and put this one test in it.

main() {
  group('Generate docs for', () {
    test('one simple file', () {
      ...
    });
  });
}

[Emily Fortuna, 2013-07-01 21:22:41]

https://codereview.chromium.org/18089006/diff/17001/pkg/docgen/test/single_li...
File pkg/docgen/test/single_library_test.dart (right):

https://codereview.chromium.org/18089006/diff/17001/pkg/docgen/test/single_li...
pkg/docgen/test/single_library_test.dart:11: test('Single file test.', () {
Disagree. Groups are only useful if you have multiple. Having one test in one
group  is unnecessary.

On 2013/07/01 19:54:29, Andrei Mouravski wrote:
> I'd recommend having a top level group and put this one test in it.
> 
> main() {
>   group('Generate docs for', () {
>     test('one simple file', () {
>       ...
>     });
>   });
> }

https://codereview.chromium.org/18089006/diff/17001/pkg/docgen/test/single_li...
pkg/docgen/test/single_library_test.dart:48:
getMirrorSystem(['test.dart']).then(expectAsync1((mirrorSystem) {
You can change this test to not use expectAsync because the future completes
with out errors. See tests/html/websql_test.dart or tests/html/xhr_test.dart for
examples of this system.

[Andrei Mouravski, 2013-07-01 21:43:52]

https://codereview.chromium.org/18089006/diff/17001/pkg/docgen/test/single_li...
File pkg/docgen/test/single_library_test.dart (right):

https://codereview.chromium.org/18089006/diff/17001/pkg/docgen/test/single_li...
pkg/docgen/test/single_library_test.dart:11: test('Single file test.', () {
On 2013/07/01 21:22:41, Emily Fortuna wrote:
> Disagree. Groups are only useful if you have multiple. Having one test in one
> group  is unnecessary.
> 
> On 2013/07/01 19:54:29, Andrei Mouravski wrote:
> > I'd recommend having a top level group and put this one test in it.
> > 
> > main() {
> >   group('Generate docs for', () {
> >     test('one simple file', () {
> >       ...
> >     });
> >   });
> > }
> 

* Disabling tests in status files requires groups.
* Cannot set setup() outside of a group.

[Emily Fortuna, 2013-07-01 21:45:16]

https://codereview.chromium.org/18089006/diff/17001/pkg/docgen/test/single_li...
File pkg/docgen/test/single_library_test.dart (right):

https://codereview.chromium.org/18089006/diff/17001/pkg/docgen/test/single_li...
pkg/docgen/test/single_library_test.dart:11: test('Single file test.', () {
On 2013/07/01 21:43:53, Andrei Mouravski wrote:
> On 2013/07/01 21:22:41, Emily Fortuna wrote:
> > Disagree. Groups are only useful if you have multiple. Having one test in
one
> > group  is unnecessary.
> > 
> > On 2013/07/01 19:54:29, Andrei Mouravski wrote:
> > > I'd recommend having a top level group and put this one test in it.
> > > 
> > > main() {
> > >   group('Generate docs for', () {
> > >     test('one simple file', () {
> > >       ...
> > >     });
> > >   });
> > > }
> > 
> 
> * Disabling tests in status files requires groups.
> * Cannot set setup() outside of a group.

I stand corrected.

[Andrei Mouravski, 2013-07-03 21:15:08]

What is the status of this CL?

[janicejl, 2013-07-03 21:35:14]

On 2013/07/03 21:15:08, Andrei Mouravski wrote:
> What is the status of this CL?

Just updated it.

[Emily Fortuna, 2013-07-03 22:14:18]

lgtm

[janicejl, 2013-07-03 22:27:45]

Committed patchset #8 manually as r24760 (presubmit successful).

[Andrei Mouravski, 2013-07-04 18:22:48]

https://codereview.chromium.org/18089006/diff/26001/pkg/docgen/test/single_li...
File pkg/docgen/test/single_library_test.dart (right):

https://codereview.chromium.org/18089006/diff/26001/pkg/docgen/test/single_li...
pkg/docgen/test/single_library_test.dart:46: getMirrorSystem(['test.dart'],'',
parseSdk: false)
Perhaps use Johnni's in-memory mirrorsystem thing here?