-
-
Notifications
You must be signed in to change notification settings - Fork 298
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.
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
Add assert helpers #436
Add assert helpers #436
Conversation
I don't really like the naming we have here. Could you think of something more description? |
👍 assertFileContent definitely reads better. |
Would you mind adding unit tests for these methods and squashing the commits togheter. Thanks you, it looks good like this. |
In further reading through the generator-ember tests I noticed that every test file defines the function I'm working on writing the unit tests now for all these methods. No tests for helpers other than |
You do not need a Generator to test these methods. You simply need to test that the assertion passes when tested against file that exist and contains the given content/regex. And that these assertion will throw when content is not within the file and/or the file exist. Just put your tests in this file, following the same style: https://github.com/yeoman/generator/blob/master/test/helpers.js |
Squashed my commits and added tests for all methods. |
assert.ok(!here, file + ' exists'); | ||
} | ||
|
||
helpers.assertFileContent = helpers.assertFile |
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.
semicolon
I think About the naming, I think Also, on a side note, I think we should refactor assertion methods for 1.0.0 so we keep them under a namespace ( |
@@ -120,6 +120,25 @@ helpers.assertFile = function (file, reg) { | |||
assert.ok(reg.test(body), file + ' did not match \'' + reg + '\'.'); | |||
}; | |||
|
|||
helpers.assertNoFile = function (file) { | |||
var here = fs.existsSync(file); | |||
assert.ok(!here, file + ' exists'); |
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.
The assert error messages should describe the expected outcome, not describe a success.
expected <file> to exists
(This is also good for the following methods)
The problem with this is that it is inconsistent with
I personally like option 2 the best. In order to avoid breaking changes, we could detect whether a regex was passed to
This is good, I'm relatively new to JS and didn't realize I could do this. Some of the resulting single lines are a little long though, does this repo have a column limit for line length?
In writing my assert error messages, I followed what was already in place ( |
I prefer |
I agree with you here. Lets do this. And you're right about the error messages, I haven't realised I read the assert No File method! |
Should the single method handling multiple args be named |
I'd go with assertFile |
OK I've updated things and merged the single/multiple functionality into |
Could you rebase on top of the lastest master? |
rebased |
Thanks! The build failed after detecting some error to our style guide. Would you mind fixing these? You can run the build locally with |
|
||
it('reject an array of file/regex pairs when one file\'s content does not matches the corresponding regex', | ||
function () { | ||
assert.throws( |
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.
These blocks would be cleaner written like this:
it('reject an array of file/regex pairs when one file\'s content does matches its corresponding regex', function () {
var arg = [
['testFile', /Roses are red/],
['testFile2', /Violets are orange/]
];
assert.throws(helpers.assertNoFileContent.bind(helpers, arg));
});
I make a last run on the functions content and leave some comment of how you can simplify some lines. I think it'd be best if the deprecated assertions methods where to Also, this is not your doing, but documentation comments aren't valid. We now use JsDoc syntax, so if you have time, it'd be really nice to convert the comments to a valid format. - But I'll merge as is if you don't have the time; so feel no obligation. |
OK, I've fixed the style errors and added JsDoc comments. In the course of adding comments to the methods I added, I decided to look at the rest of the helpers. I added JsDoc style comments and made a some internal changes to a few of the helpers:
A few questions/comments:
|
Just let me know if it looks good and I'll squash/rebase |
This is a very solid PR. Awesome work dude! I'm very happy with the efforts you put in. Thanks a lot! 🌠
I agree. Could you replace the name, but alias
That seems to be wrong. Can you find it used anywhere in our code base? (is there unit test showing how the method is used?) |
refactor some other helpers
Done.
The only place it's used in this repo is in The build is failing right now and I'm not sure why. On my machine, |
Yeah, tests fails because seems like Coveralls returns invalid JSON when it refuse a coverage reports... Anyway, I'll handle this later on. Thanks a lot! |
While correcting an issue over at generator-ember, the need came up for a test to make sure that compass was not installed when the user declines to install compass at the prompt. There was no convenient helper method available to assert the absence of a regex match (needed here for _package.json and Gruntfile.js), so I added it under the name
assertNoMatch
. It also seemed as if the regex form ofassertFile
should be aliased toassertMatch
for symmetry.