Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

migrate testing from qunit to mocha #1531

Merged
merged 1 commit into from
Feb 24, 2015
Merged

migrate testing from qunit to mocha #1531

merged 1 commit into from
Feb 24, 2015

Conversation

patrickkettner
Copy link
Member

No description provided.

@patrickkettner
Copy link
Member Author

I almost have the sauce job working - there is an issue with selenium on saucelabs on Opera 12 that I am speaking with their support about, and there are a few timeout issues with old IE. Also a few caniuse mismatches on a couple browsers.

@patrickkettner patrickkettner mentioned this pull request Feb 22, 2015
Closed
@patrickkettner patrickkettner force-pushed the update-tests branch 3 times, most recently from e3ebd4c to d5dd105 Compare February 22, 2015 07:31
ryanseddon
ryanseddon reviewed Feb 22, 2015
View reviewed changes
Gruntfile.js
integrationTests: grunt.file.expand(['test/browser/integration/*.js'])
}
}),
function(req, rep, next) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit picky, but my brain had to pause for a second to understand what rep was I think res is more obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

ryanseddon
ryanseddon reviewed Feb 23, 2015
View reviewed changes
test/browser/integration/global.js
@@ -0,0 +1,7 @@
describe('basics', function() {

it('createa a global modernizr object', function() {
Copy link
Member

Choose a reason for hiding this comment

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

createa a -> creates a

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

ryanseddon
ryanseddon reviewed Feb 23, 2015
View reviewed changes
test/browser/integration/prefixed.js
len;

if(!obj) {
if (prop in elem.style)
Copy link
Member

Choose a reason for hiding this comment

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

Should we just stick to a single if convention an require curly braces all the time?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, most definitely. almost all of integration was copied over fairly blindly.
I'll clean up what I see, and after this merges I will work on getting some jscs chocolate all up in our peanut butter

Copy link
Member Author

Choose a reason for hiding this comment

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

updated everything I saw that was without braces

@patrickkettner patrickkettner force-pushed the update-tests branch 2 times, most recently from e1db29c to a2b8f88 Compare February 23, 2015 00:44
ryanseddon
ryanseddon reviewed Feb 23, 2015
View reviewed changes
test/browser/src/injectElementWithStyles.js
expect(result).to.be(true);
});

it('passses back a rule matching what we gave it', function(done) {
Copy link
Member

Choose a reason for hiding this comment

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

one too many s's

Copy link
Member Author

Choose a reason for hiding this comment

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

fixeddd

ryanseddon
ryanseddon reviewed Feb 23, 2015
View reviewed changes
test/browser/src/isEventSupported.js
});

it('returns false when the event does not exists', function() {
expect(isEventSupported('fart')).to.be(false);
Copy link
Member

Choose a reason for hiding this comment

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

I laughed at this way more than a grown adult should

Copy link
Member Author

Choose a reason for hiding this comment

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

taint the only one ;)

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 noticed the consistency of fart throughout.

Copy link
Member Author

Choose a reason for hiding this comment

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

trying to keep it profesh

ryanseddon
ryanseddon reviewed Feb 23, 2015
View reviewed changes
test/browser/src/prefixed.js
expect(testPropsAll.calledOnce).to.be(true);
});

it('uses looks up properties on an element when provided', function() {
Copy link
Member

Choose a reason for hiding this comment

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

this reads weird

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

perfect

ryanseddon
ryanseddon reviewed Feb 23, 2015
View reviewed changes
test/node/lib/build.js
});
});

it('should stip out DOC comments when `uglify`ing', function(done) {
Copy link
Member

Choose a reason for hiding this comment

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

stip -> strip

@ryanseddon
Copy link
Member

This is an amazing effort!

ryanseddon
ryanseddon reviewed Feb 23, 2015
View reviewed changes
test/browser/src/setClasses.js
after(teardown);

it('adds a class without a prefix', function(done) {

Copy link
Member

Choose a reason for hiding this comment

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

Actually I meant this one

@patrickkettner
Copy link
Member Author

after the static changes are made, are you 👍?

@ryanseddon
Copy link
Member

yep 👍

@patrickkettner patrickkettner force-pushed the update-tests branch 4 times, most recently from ca5c033 to ca3271e Compare February 23, 2015 19:31
@patrickkettner patrickkettner force-pushed the update-tests branch 6 times, most recently from b540a53 to 31098e3 Compare February 23, 2015 21:48
@patrickkettner
Copy link
Member Author

@ryanseddon wanna give it a quick once over? Static building is in now. You can see an example at patrickkettner.github.io/Modernizr/test/

Basically, on successful merge to master, the gh-pages branch is updated with the output of grunt copy:gh-pages

huge thanks to @alrra for showing me that and getting it working

@ryanseddon
Copy link
Member

Yep looks good, so grunt copy:gh-pages will just be ran manually after any changes to master?

Also appveyor failed due to phantomjs timing out, is this a blocker for merging?

@ryanseddon
Copy link
Member

Also seeing 4 tests failing on Fx35

@patrickkettner
Copy link
Member Author

No, it runs automatically on Travis. We don't have to manually do anything
ever again

On Mon, Feb 23, 2015, 9:42 PM Ryan Seddon notifications@github.com wrote:

Also seeing 4 tests failing on Fx35

—
Reply to this email directly or view it on GitHub
#1531 (comment).

@patrickkettner
Copy link
Member Author

@ryanseddon what tests are you seeing failed on what system? I am all green on FF35 on Yosemite

@patrickkettner patrickkettner force-pushed the update-tests branch 2 times, most recently from e9240bf to ad975cc Compare February 24, 2015 04:26
@patrickkettner
Copy link
Member Author

@ryanseddon able to reproduce, weird caching thing I guess. One issue is related to this, three are not. I was calling !!bool rather than bool.valueOf(), which meant that new Boolean(false) objects where coming back as true. Still debugging appveyor.

patrickkettner added a commit that referenced this pull request Feb 24, 2015
@patrickkettner
Merge pull request #1531 from patrickkettner/update-tests
7d19f06 
migrate testing from qunit to mocha
@patrickkettner patrickkettner merged commit 7d19f06 into Modernizr:master Feb 24, 2015
@patrickkettner patrickkettner deleted the update-tests branch February 24, 2015 05:34
@SlexAxton
Copy link
Member

Magic. You two.

@patrickkettner
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@patrickkettner @ryanseddon @SlexAxton