-
Notifications
You must be signed in to change notification settings - Fork 2
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Made a start on writing some tests. They're currently failing though …
…– testing for an expection when submitting with an API key – not sure what I should be checking for here...
- Loading branch information
Oliver Farrell
committed
May 25, 2015
1 parent
4294f23
commit c0854be
Showing
2 changed files
with
64 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
/** | ||
* Command Line Tests | ||
*/ | ||
|
||
var mocha = require('mocha'), | ||
chai = require('chai'), | ||
expect = chai.expect, | ||
perfbudget = require('../perfbudget.js'), | ||
cli = require('../lib/normalizeCli.js'); | ||
|
||
describe('perfbudget', function() { | ||
|
||
// set timeout to account for time taken to get results | ||
this.timeout(60000); | ||
|
||
it('should fail if no API Key is provided', function () { | ||
var options = { | ||
"url": "http://www.bbc.co.uk", | ||
"key": "", | ||
"location": "Dulles:Chrome", | ||
"wptInstance": "www.webpagetest.org", | ||
"video": 1, | ||
"runs": 1, | ||
"pollResults": 5, | ||
"timeout": 60, | ||
"repeatView": false, | ||
"budget": { | ||
"render": "1000", | ||
"SpeedIndex": "1000" | ||
} | ||
}; | ||
|
||
expect(function() { | ||
perfbudget.runTest('123', 0); | ||
}).to.throw(); | ||
}); | ||
|
||
// should return metrics | ||
it('should return results for requested metrics', function (done) { | ||
var options = { | ||
"url": "http://www.bbc.co.uk", | ||
"key": "", | ||
"location": "Dulles:Chrome", | ||
"wptInstance": "www.webpagetest.org", | ||
"video": 1, | ||
"runs": 1, | ||
"pollResults": 5, | ||
"timeout": 60, | ||
"repeatView": false, | ||
"budget": { | ||
"render": "1000", | ||
"SpeedIndex": "1000" | ||
} | ||
}; | ||
|
||
perfbudget.runTest(options, function (err) { | ||
if (err) throw err; | ||
done(); | ||
}); | ||
}); | ||
}); |
c0854be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliverfarrell
I think this is super hard to test, because
cli.js
is so tightly coupled toperfbudget.js
and can not think of any sane solution to test this. But it's doing one function call and some logging.I think we can live without that.
But I'd write tests for
normalizeCli
andperfbudget
totally encapsulated.The code might need some refactoring to make it possible to mock the Webpagetest-API. We shouldn't test code that doesn't belong to us ( this is strongly opinionated ).
What do you think?
c0854be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefanjudis
I must confess I'm still getting to grips with the concept of writing tests – it's quite possible I might be going about some of this a little wrong.
I'm about to push an update to reflect your comments. I'm renaming the
/test/cli.js
file to/test/perfbudget.js
and have put the"should return results for requested metrics"
test inside that which when I supply it with an API key, passes the test. As all of these tests would require an API key is this why you suggest mocking the WebPageTest-API to avoid us having to store an API key inside the tests?Inside
/test/normalizeCli.js
I should then be testing that each flag works as expected, would that be correct?Forgive me for being a bit of a newb with this :)