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.

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

feat: add code and tests to allow for testing errors #149

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DesignByOnyx
Copy link

@DesignByOnyx DesignByOnyx commented Jun 22, 2019

This allows for testing that an @error directive was triggered. This is best illustrated by the errors test file.

By default, all assert-error calls 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):

  1. Replace the string assert-error with _assert_error_execute
    • this will actually render the @content of the assert-error block, which should trigger the error
  2. Run sass.renderSync inside of a try/catch block
    • if no error occurs, then this is a failure. Replace _assert_error_execute with _assert_error_fail. Under the hood this calls assert-equal('Expected an error to be thrown', 'No error thrown')
  3. Inside the catch block, detect if there is an expected message
    • if the emitted message does not match what was expected, this is a failure. Replace _assert_error_execute with _assert_error_fail. Under the hood this calls assert-equal($actual_message, $expected_message)
    • if an error was emitted and the expected message matches, then this is a success. Replace _assert_error_execute with _assert_error_success. Under the hood this calls assert-true(true).

After the preprocessing is done, all of the assert-error strings will have been replaced with either _assert_error_success or _assert_error_fail, both of which simply call normal assertions which will be reported by the True parser. The newly formatted string is then passed to sass.renderSync and the True engine works as it normally would. Hope that makes sense.

Fixes #146

@coveralls
Copy link

coveralls commented Jun 22, 2019

Pull Request Test Coverage Report for Build 458

  • 14 of 23 (60.87%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-3.07%) to 96.934%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/main.js 14 23 60.87%
Totals Coverage Status
Change from base Build 454: -3.07%
Covered Lines: 306
Relevant Lines: 315

💛 - Coveralls

Copy link
Member

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

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

This works as expected when the @include assert-error is in the file directly passed into True, but does nothing if the assertion is in a nested (@imported) file.

lib/main.js Show resolved Hide resolved
sassOpts.data = fs.readFileSync(sassOpts.file, 'utf-8');
sassOpts.includePaths.push(path.dirname(sassOpts.file));
delete sassOpts.file;
}
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mirisuzanne mirisuzanne left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I like this idea, but I think there's still cleanup that needs to happen.

/// @include some-mixin($bad-params);
/// }
/// }
@mixin assert-error($message: null) {
Copy link
Member

Choose a reason for hiding this comment

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

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 */
/*  */

Copy link
Author

Choose a reason for hiding this comment

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Author

@DesignByOnyx DesignByOnyx Jul 15, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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') {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 thrown
  • assert-error-matches(msg): passes if the msg equals the thrown error message
  • assert-error-contains(msg): passes if the msg appears within the thrown error message

@jgerigmeyer jgerigmeyer changed the base branch from master to main July 17, 2020 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Support for intended fail-scenario
4 participants