Skip to content

Commit

Permalink
test(WebPlatform): disable web components tests when shim is needed
Browse files Browse the repository at this point in the history
  • Loading branch information
@vicb
vicb committed Aug 30, 2014
1 parent cb40e76 commit e9cd782
Showing 1 changed file with 13 additions and 1 deletion.
14 changes: 13 additions & 1 deletion test/core_dom/web_platform_spec.dart
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
library angular.dom.platform_spec;

import '../_specs.dart';
import 'package:browser_detect/browser_detect.dart';

import 'dart:js' as js;

main() {
describe('WebPlatform', () {

beforeEachModule((Module module) {
module
..bind(_WebPlatformTestComponent)
Expand All @@ -19,6 +19,10 @@ main() {
it('should scope styles to shadow dom across browsers.',
async((TestBed _, MockHttpBackend backend, WebPlatform platform) {

// TODO(vicb) WebPlatform does not work with polyfills

This comment has been minimized.

Sign in to view

Copy link
@vsavkin

vsavkin Sep 2, 2014

Do we have to skip this test? The components used in this test use tag selectors, so it is not clear how they are affected by dart-archive#1300.

This comment has been minimized.

Sign in to view

Copy link
@vicb

vicb Sep 4, 2014

Author Owner

I haven't look at the details of the tests.
I don't see a need to test anything here as WP does not work on angular yet (general case)

This comment has been minimized.

Sign in to view

Copy link
@vsavkin

vsavkin Sep 4, 2014

The only limitation of WP I'm aware of is that it supports only tag selectors. This limitation, however, is captured by the next test: "should not crash with an attribute selector; but wont work either..". So imo the two tests:

  • should scope styles to shadow dom across browsers.
  • should not crash with an attribute selector; but wont work either..

actually describe the current behavior in Chrome and Firefox.

There might be another problem with WP that causes these tests to fail in other browsers. In this case we should probably change the comment, cause right now it is misleading.

I think these tests makes sense only when the shim is required. If the shim is not required, WP is a noop, so I'm not sure what we are testing.

This comment has been minimized.

Sign in to view

Copy link
@vicb

vicb Sep 4, 2014

Author Owner

So I should un-comment the 2 following tests:

  • should scope styles to shadow dom across browsers.
  • should not crash with an attribute selector; but wont work either..

I think these tests makes sense only when the shim is required.

Unit tests should not be different whether the target browser needs / doesn't need shim. I think I once entered a defect with such a remark.

This comment has been minimized.

Sign in to view

Copy link
@vicb

vicb Sep 4, 2014

Author Owner

I've re-enable the 2 tests, If they crash, I'll disable them again and create an issue for this to be fixed separately.

This comment has been minimized.

Sign in to view

Copy link
@vsavkin

vsavkin Sep 4, 2014

Thank you
// see https://github.com/angular/angular.dart/issues/1300
if (platform.cssShimRequired) return;

backend
..expectGET('style.css').respond(200, 'span { background-color: red; '
'}')
Expand Down Expand Up @@ -58,6 +62,10 @@ main() {
it('should not crash with an attribute selector; but wont work either..',
async((TestBed _, MockHttpBackend backend, WebPlatform platform) {

// TODO(vicb) WebPlatform does not work with polyfills
// see https://github.com/angular/angular.dart/issues/1300
if (platform.cssShimRequired) return;

backend
..expectGET('style.css').respond(200, 'span { background-color: red; '
'}')
Expand All @@ -76,6 +84,10 @@ main() {
it('should scope :host styles to the primary element.',
async((TestBed _, MockHttpBackend backend, WebPlatform platform) {

// TODO(vicb) WebPlatform does not work with polyfills
// see https://github.com/angular/angular.dart/issues/1300
if (platform.cssShimRequired) return;

backend
..expectGET('style.css').respond(200, ':host {'
'background-color: red; }')
Expand Down

0 comments on commit e9cd782

Please sign in to comment.