feat: add code and tests to allow for testing errors #149
Conversation
DesignByOnyx
force-pushed
the
146-assert-error
branch
from
d0f6b7d
to
c2c3e9f
Compare
Pull Request Test Coverage Report for Build 458
💛 - Coveralls |
| sassOpts.data = fs.readFileSync(sassOpts.file, 'utf-8'); | ||
| sassOpts.includePaths.push(path.dirname(sassOpts.file)); | ||
| delete sassOpts.file; | ||
| } |
There was a problem hiding this comment.
This approach no longer works if the test is in an imported file, not the parent file being passed in. I'm not sure how to get around that without re-implementing the Sass import logic?
There was a problem hiding this comment.
This is related to your comment above regarding assertions nested within @imported files. I will test this in my attempts to fix the greater problem of importing test files.
| /// @include some-mixin($bad-params); | ||
| /// } | ||
| /// } | ||
| @mixin assert-error($message: null) { |
There was a problem hiding this comment.
I think we can improve messaging in the plain-CSS compile. Right now if you run yarn compile or yarn commit, this is the output:
/* # Module: Assert Error */
/* ---------------------- */
/* Test: Detects when an error is thrown */
/* */
/* Test: Compares a partial message to the emitted message (case-insensitive) */
/* */
/* Test: Compares the full message to the emitted message (case-insensitive) */
/* */
/* */I'd like to see something more like:
/* Test: Compares the full message to the emitted message (case-insensitive) */
/* ? [assert-error] Must pass a valid color */
/* */There was a problem hiding this comment.
Thanks for the feedback. I'll see what I can do. I was not evaluating the final css.
| /// } | ||
| /// } | ||
| @mixin assert-error($message: null) { | ||
| @if (false) { |
There was a problem hiding this comment.
I still think it would be useful for manual testing to allow a variable override to be passed in – and setting that variable also seems simpler than search-replace of the entire mixin call. Since a global variable could be passed to all mixins, that could potentially also help resolve @jgerigmeyer's note about testing imported files?
There was a problem hiding this comment.
I am not entirely opposed to this. However, this introduces some complexities which might send a user on a wild red herring chase. For example, let's say a user wants to manually test an error by passing true, there could another unrelated error elsewhere in the file. Users might think that their error code is being hit when in fact it is not.
Regarding the global variable thing... due to the fact that errors are fatal, we must still do one-by-one replacements for error assertions as they must be tested one at a time. There's no way to set a single global variable but only have it affect one instance of a mixin. Could you maybe provide some sample code on how you think a global variable would work as I might not be understanding you correctly.
All this said, I'll go ahead and add a second parameter so you can test it and see how it feels. If you want it to stay, I'm all for it.
There was a problem hiding this comment.
Those are great points, makes sense.
| } | ||
|
|
||
| @include test('Compares the full message to the emitted message (case-insensitive)') { | ||
| @include assert-error('mUst PasS a vAliD cOloR') { |
There was a problem hiding this comment.
I can't get these tests to fail.
- I commented out the error above, and it still passed
- I changed the message completely, and it still passed
I'd also want to discuss more if case-insensitivity and partial-matches are the right way to go. We don't support the former anywhere else in True, and we only support the latter when explicitly requested using the contains() mixin.
There was a problem hiding this comment.
Thanks. I figured this would be a point of discussion. I think I would prefer then to see something for different scenarios:
assert-throws(): passes if any error is thrownassert-error-matches(msg): passes if the msg equals the thrown error messageassert-error-contains(msg): passes if the msg appears within the thrown error message
This allows for testing that an
@errordirective was triggered. This is best illustrated by the errors test file.By default, all
assert-errorcalls are no-ops - it literally does nothing. The way we test the error assertions is by pre-parsing the input file/data:For each call to
assert-error, do the following (one at a time):assert-errorwith_assert_error_execute@contentof theassert-errorblock, which should trigger the errorsass.renderSyncinside of a try/catch block_assert_error_executewith_assert_error_fail. Under the hood this callsassert-equal('Expected an error to be thrown', 'No error thrown')catchblock, detect if there is an expected message_assert_error_executewith_assert_error_fail. Under the hood this callsassert-equal($actual_message, $expected_message)_assert_error_executewith_assert_error_success. Under the hood this callsassert-true(true).After the preprocessing is done, all of the
assert-errorstrings will have been replaced with either_assert_error_successor_assert_error_fail, both of which simply call normal assertions which will be reported by theTrueparser. The newly formatted string is then passed tosass.renderSyncand theTrueengine works as it normally would. Hope that makes sense.Fixes #146