Skip to content

Conversation

rxaviers
Copy link
Member

@rxaviers rxaviers commented May 8, 2013

Discussion on #951

@rxaviers
Copy link
Member Author

rxaviers commented May 8, 2013

@mikesherov can you help me by double checking if there's really nothing to do on the unit tests side? As far as I saw, the scale-test tests scale only (not puff or size explicitly).

@scottgonzalez
Copy link
Member

Thanks @rxaviers. This seems right to me. I created a ticket for this; we'll need to update the commit message to reference it.

This is probably a good time to add unit tests. @gnarf37 @mikesherov Can you do a review of this as well? Splitting files always seems like something that should get lots of eyes :-)

@jzaefferer
Copy link
Member

I checked out the branch and ran tests/unit/effects/effects.html - it got stuck while running effects.scale: show/hide.

At least all effects are covered with the generated tests in effects_core.js.

@rxaviers
Copy link
Member Author

effects_core fixed. Now, grunt test executes just like before. Note that I have searched for puff or size references in the test files in order to find those fix spots. Still good to have more eyeballs on those changes.

if ( effect !== "puff" && effect !== "size" ) {
TestHelpers.testJshint( "effect-" + effect );
}
console.log("testJshint", "effect-", effect);
Copy link
Member

Choose a reason for hiding this comment

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

remove this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

ouch! removed

@mikesherov
Copy link
Member

Other than removing the console log, I approve of this. 👍

@rxaviers
Copy link
Member Author

When building manifest, I've noticed the new puff and scale dependencies were missing. So, I've updated its manifest data (amended commit).

@rxaviers
Copy link
Member Author

Landed d0c613d and 1f4f002

@rxaviers rxaviers closed this Jun 22, 2013
@rxaviers rxaviers deleted the scale-size-puff-effects branch June 22, 2013 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants