Add resource tests.

Add resource tests.

R=sgjesse@google.com, whesse@google.com

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

[Lasse Reichstein Nielsen, 2015-08-07 12:21:34]

lrn@google.com changed reviewers:
+ iposva@chromium.org, sgjesse@google.com

[Lasse Reichstein Nielsen, 2015-08-07 12:21:35]

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
File tests/corelib/data_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
tests/corelib/data_resource_test.dart:13: // await
testUri("data:text/plain;charset=utf-8;base64,$base64Encoded");
I think we should properly support the data: scheme for resources:
- don't require application/dart for the content type (since it likely isn't for
resources)
- accept base64 encoding.

[Søren Gjesse, 2015-08-07 13:51:16]

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
File tests/corelib/data_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
tests/corelib/data_resource_test.dart:10: // await
testUri("data:text/plain;charset=utf-8,$uriEncoded");
Open bugs on these contentType and base64 issues.

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
tests/corelib/data_resource_test.dart:40: if (!compareBytes(streamBytes, bytes))
{
I don't think this check adds anything.

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/file_resource...
File tests/corelib/file_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/file_resource...
tests/corelib/file_resource_test.dart:36: if (!compareBytes(streamBytes, bytes))
{
ditto.

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/http_resource...
File tests/corelib/http_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/http_resource...
tests/corelib/http_resource_test.dart:35: if (!compareBytes(streamBytes, bytes))
{
ditto.

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/package_resou...
File tests/corelib/package_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/package_resou...
tests/corelib/package_resource_test.dart:8: const uriText =
"package:expect/resources/sample.txt";
Where does this resource come from?

[Lasse Reichstein Nielsen, 2015-08-07 13:55:52]

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
File tests/corelib/data_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
tests/corelib/data_resource_test.dart:40: if (!compareBytes(streamBytes, bytes))
{
True, it made sense for the package_resource_test because that file may contain
a line terminator, but here it isn't necessary.

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/package_resou...
File tests/corelib/package_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/package_resou...
tests/corelib/package_resource_test.dart:8: const uriText =
"package:expect/resources/sample.txt";
I just added it in this CL.
The alternative is to add a new test_samples package, which might be prettier
than adding stuff to Expect.

[Ivan Posva, 2015-08-10 05:39:56]

iposva@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2015-08-10 05:39:56]

Thanks,
-Ivan

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
File tests/corelib/data_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
tests/corelib/data_resource_test.dart:13: // await
testUri("data:text/plain;charset=utf-8;base64,$base64Encoded");
On 2015/08/07 12:21:35, Lasse Reichstein Nielsen wrote:
> I think we should properly support the data: scheme for resources:
> - don't require application/dart for the content type (since it likely isn't
for
> resources)
ACK

> - accept base64 encoding.
Once the Uri class understands base64 encoding/decoding, then that should become
simple in the implementations.

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/http_resource...
File tests/corelib/http_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/http_resource...
tests/corelib/http_resource_test.dart:55: var response = request.response;
Don't you want to verify that the request is made for the correct resource? The
way it is written currently it would not fail if the request was made to fetch a
file named "txt.elpmas".

[Lasse Reichstein Nielsen, 2015-08-10 10:40:09]

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
File tests/corelib/data_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
tests/corelib/data_resource_test.dart:10: // await
testUri("data:text/plain;charset=utf-8,$uriEncoded");
On 2015/08/07 13:51:16, Søren Gjesse wrote:
> Open bugs on these contentType and base64 issues.

Acknowledged.

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/data_resource...
tests/corelib/data_resource_test.dart:13: // await
testUri("data:text/plain;charset=utf-8;base64,$base64Encoded");
I'm not sure the Uri class is the right place - but then again, that's where the
data:-URI ends up anyway, even if they aren't hierarchical.
That'll need some design though to not mess up :)

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/http_resource...
File tests/corelib/http_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/http_resource...
tests/corelib/http_resource_test.dart:55: var response = request.response;
Good idea.

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/package_resou...
File tests/corelib/package_resource_test.dart (right):

https://codereview.chromium.org/1276263002/diff/1/tests/corelib/package_resou...
tests/corelib/package_resource_test.dart:8: const uriText =
"package:expect/resources/sample.txt";
I've created a package_test_data/ package.

[Lasse Reichstein Nielsen, 2015-08-10 10:41:50]

lrn@google.com changed reviewers:
+ whesse@google.com
- iposva@chromium.org

[Lasse Reichstein Nielsen, 2015-08-10 10:41:50]

lrn@google.com changed required reviewers:
+ sgjesse@google.com

[Lasse Reichstein Nielsen, 2015-08-10 10:41:51]

+Bill for test suite change.

https://codereview.chromium.org/1276263002/diff/20001/tools/testing/dart/test...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/1276263002/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_suite.dart:887: 
Is this actually necessary?

[Bill Hesse, 2015-08-10 11:37:43]

lgtm

https://codereview.chromium.org/1276263002/diff/20001/tools/testing/dart/test...
File tools/testing/dart/test_suite.dart (right):

https://codereview.chromium.org/1276263002/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_suite.dart:887: 
On 2015/08/10 10:41:51, Lasse Reichstein Nielsen wrote:
> Is this actually necessary?

No, this is not needed.  Only those tests that are in a package themselves get a
package_root that is constructed by calling this.
Since there is no packages.yaml in the directory containing the tests, or above
that dir, they don't get a special package directory when these options
(--use-repository-packages or --use-public-packages) are used.

https://codereview.chromium.org/1276263002/diff/20001/tools/testing/dart/test...
tools/testing/dart/test_suite.dart:911: testDataLink.toNativePath(),
testDataLinkTarget.toNativePath()));
Also not needed.

[Bill Hesse, 2015-08-10 11:38:55]

LGTM only on the test_suite.dart, which the changes removed, which is
effectively "!LGTM, remove these changes".

On 2015/08/10 11:37:43, Bill Hesse wrote:
> lgtm
> 
>
https://codereview.chromium.org/1276263002/diff/20001/tools/testing/dart/test...
> File tools/testing/dart/test_suite.dart (right):
> 
>
https://codereview.chromium.org/1276263002/diff/20001/tools/testing/dart/test...
> tools/testing/dart/test_suite.dart:887: 
> On 2015/08/10 10:41:51, Lasse Reichstein Nielsen wrote:
> > Is this actually necessary?
> 
> No, this is not needed.  Only those tests that are in a package themselves get
a
> package_root that is constructed by calling this.
> Since there is no packages.yaml in the directory containing the tests, or
above
> that dir, they don't get a special package directory when these options
> (--use-repository-packages or --use-public-packages) are used.
> 
>
https://codereview.chromium.org/1276263002/diff/20001/tools/testing/dart/test...
> tools/testing/dart/test_suite.dart:911: testDataLink.toNativePath(),
> testDataLinkTarget.toNativePath()));
> Also not needed.

[Søren Gjesse, 2015-08-10 12:22:18]

lgtm

https://codereview.chromium.org/1276263002/diff/20001/pkg/package_test_data/p...
File pkg/package_test_data/pubspec.yaml (right):

https://codereview.chromium.org/1276263002/diff/20001/pkg/package_test_data/p...
pkg/package_test_data/pubspec.yaml:2: version: 0.0.1-dev
I guess remove the -dev here.

https://codereview.chromium.org/1276263002/diff/20001/pkg/package_test_data/p...
pkg/package_test_data/pubspec.yaml:6: Should not be published.
*not*

[Lasse Reichstein Nielsen, 2015-08-11 07:53:53]

https://codereview.chromium.org/1276263002/diff/20001/pkg/package_test_data/p...
File pkg/package_test_data/pubspec.yaml (right):

https://codereview.chromium.org/1276263002/diff/20001/pkg/package_test_data/p...
pkg/package_test_data/pubspec.yaml:2: version: 0.0.1-dev
It's basically meant as a marker for "this has not been published".

https://codereview.chromium.org/1276263002/diff/20001/pkg/package_test_data/p...
pkg/package_test_data/pubspec.yaml:6: Should not be published.
On 2015/08/10 12:22:18, Søren Gjesse wrote:
> *not*

Done.

[Lasse Reichstein Nielsen, 2015-08-11 08:07:14]

Committed patchset #3 (id:40001) manually as
c26095b1b30f4eadee56a15118191ef9f85e0269 (presubmit successful).

[gzoechi, 2015-08-11 08:33:49]

gzoechi@gmail.com changed reviewers:
+ gzoechi@gmail.com

[gzoechi, 2015-08-11 08:33:49]

I think adding 

   publish_to: none

to pubspec.yaml would be better than adding "don't publish" to the description

[Lasse Reichstein Nielsen, 2015-08-11 09:20:13]

On 2015/08/11 08:33:49, gzoechi wrote:
> I think adding 
> 
>    publish_to: none
> 
> to pubspec.yaml would be better than adding "don't publish" to the description

Thanks, did not know about that one (it's not on
https://www.dartlang.org/tools/pub/pubspec.html).