Skip to content

Commit

Permalink
Conditionally run height/width in iframeTest
Browse files Browse the repository at this point in the history
  • Loading branch information
@rwaldron
rwaldron committed Jun 6, 2012
1 parent 53a5810 commit 5d25f78
Showing 1 changed file with 7 additions and 2 deletions.
9 changes: 7 additions & 2 deletions test/unit/dimensions.js
View file
Expand Up @@ -430,8 +430,13 @@ testIframe( "dimensions/documentSmall", "window vs. small document", function( j
testIframe( "dimensions/documentLarge", "window vs. large document", function( jQuery, window, document ) {
expect(2);

ok( jQuery( document ).height() > jQuery( window ).height(), "document height is larger than window height" );
ok( jQuery( document ).width() > jQuery( window ).width(), "document width is larger than window width" );
if ( jQuery.fn.height && jQuery.fn. width ) {

This comment has been minimized.

Sign in to view

Copy link
@staabm

staabm Jun 7, 2012

Contributor

Space in front of width

This comment has been minimized.

Sign in to view

Copy link
@rwaldron

rwaldron Jun 7, 2012

Author Member

Please be sure to review the latest version of the code before providing review contributions - this has already been fixed (ie. removed)
expect(2);
ok( jQuery( document ).height() > jQuery( window ).height(), "document height is larger than window height" );
ok( jQuery( document ).width() > jQuery( window ).width(), "document width is larger than window width" );
} else {
expect(0);
}
});

}

14 comments on commit 5d25f78

@Krinkle
Copy link
Member

@Krinkle Krinkle commented on 5d25f78 Jun 6, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 tests is considered a failure by TestSwarm. Also there is another expect(2); higher in the function. Is there some context behind this?

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The context is that if dimensions isn't built (it's now conditional), then height and width functions are defined.

@Krinkle
Copy link
Member

@Krinkle Krinkle commented on 5d25f78 Jun 6, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I missed that new feature. Awesome.

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this commit. Why does this matter if the whole module is wrapped in this same conditional you've added?

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know what to tell you... There were errors, this fixed it. I say we just move on, shall we?

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd love to move on, but this indicates that I can't reliably test locally, because those conditionals weren't necessary locally, nor do they really make any sense, unless I don't know how testswarm works.

@dmethvin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwldrn was that failing for you locally? I'm not sure why you'd need to have another guard test but maybe it was a caching issue.

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Locally, yes. No cache, I test in incognito mode with cache off and double down with cache busted URLs.

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just figured it had something to do with running in an iframe.

@mikesherov
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just admit that this commit makes no sense and we can move on :-)

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo, I'll admit that.

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may never know why I saw what I saw, but it is no more. Fresh build -dimensions, no issues now...

@Krinkle
Copy link
Member

@Krinkle Krinkle commented on 5d25f78 Jun 7, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikesherov The conditional has no relation to testswarm. The issue with testswarm is the expect(0);. If a test should not be run, then conditionally run the test instead of just its assertions. If a test has 0 assertions, it is considered a failure.

@rwaldron
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Krinkle have we figured out a way to test optional module builds? The expect(0) code can only be reached if jQuery was built with: grunt build:*:*:-dimensions

Please sign in to comment.