Test invalid contents for importer results (#1834)

* Test invalid arguments for importer results.
* Tests contents not being string values
* Tests SassException argument errors have better output
* Adds sandbox directory to .gitignore
* Also test legacy API
* add eslint-ignore lines for  because we are testing invalid typings
* don't use sandbox

.gitignore

@@ -3,3 +3,4 @@ tmp
 .DS_Store
 Gemfile.*.lock
 node_modules
+spec/sandbox

js-api-spec/importer.test.ts

@@ -3,7 +3,6 @@
 // https://opensource.org/licenses/MIT.
 
 import {URL} from 'url';
-
 import {compile, compileString, compileStringAsync, Importer} from 'sass';
 
 import {sandbox} from './utils';
@@ -624,7 +623,7 @@ describe('FileImporter', () => {
       }));
 
     it('wraps an error', async () => {
-      await expect(() =>
+      expect(() =>
         compileStringAsync('@import "other";', {
           importers: [
             {
@@ -660,6 +659,81 @@ it(
     })
 );
 
+describe('when importer does not return string contents', () => {
+  it('throws an error in sync mode', () => {
+    expect(() => {
+      compileString('@import "other";', {
+        importers: [
+          {
+            canonicalize: url => new URL(`u:${url}`),
+            load() {
+              return {
+                // Need to force an invalid type to test bad-type handling.
+                // eslint-disable-next-line @typescript-eslint/no-explicit-any
+                contents: Buffer.from('not a string') as any,
+                syntax: 'scss',
+              };
+            },
+          },
+        ],
+      });
+    }).toThrowSassException({
+      line: 0,
+      includes:
+        'Invalid argument (contents): must be a string but was: Buffer: ' +
+        "Instance of 'NativeUint8List'",
+    });
+  });
+
+  it('throws an error in async mode', async () => {
+    expect(async () => {
+      await compileStringAsync('@import "other";', {
+        importers: [
+          {
+            canonicalize: url => new URL(`u:${url}`),
+            load() {
+              return {
+                // Need to force an invalid type to test bad-type handling.
+                // eslint-disable-next-line @typescript-eslint/no-explicit-any
+                contents: Buffer.from('not a string') as any,
+                syntax: 'scss',
+              };
+            },
+          },
+        ],
+      });
+    }).toThrowSassException({
+      line: 0,
+      includes:
+        'Invalid argument (contents): must be a string but was: Buffer: ' +
+        "Instance of 'NativeUint8List'",
+    });
+  });
+});
+
+it('throws an ArgumentError when the result sourceMapUrl is missing a scheme', () => {
+  expect(() => {
+    compileString('@import "other";', {
+      importers: [
+        {
+          canonicalize: url => new URL(`u:${url}`),
+          load() {
+            return {
+              contents: '',
+              syntax: 'scss',
+              sourceMapUrl: {} as URL,
+            };
+          },
+        },
+      ],
+    });
+  }).toThrowSassException({
+    line: 0,
+    includes:
+      "Invalid argument (sourceMapUrl): must be absolute: Instance of '_Uri'",
+  });
+});
+
 /**
  * Returns an importer that asserts that `fromImport` is `expected`, and
  * otherwise imports exclusively empty stylesheets.

js-api-spec/legacy/importer.test.ts

@@ -859,3 +859,53 @@ describe('render()', () => {
       done();
     }));
 });
+
+describe('when importer returns non-string contents', () => {
+  it('throws an error in sync mode', () => {
+    expect(() => {
+      sass.renderSync({
+        data: '@import "other";',
+        importer() {
+          return {
+            // Need to force an invalid type to test bad-type handling.
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            contents: Buffer.from('not a string') as any,
+            syntax: 'scss',
+          };
+        },
+      });
+    }).toThrowLegacyException({
+      line: 1,
+      includes:
+        'Invalid argument (contents): must be a string but was: Buffer: ' +
+        "Instance of 'NativeUint8List'",
+    });
+  });
+
+  it('throws an error in async mode', done => {
+    sass.render(
+      {
+        data: '@import "other";',
+        importer() {
+          return {
+            // Need to force an invalid type to test bad-type handling.
+            // eslint-disable-next-line @typescript-eslint/no-explicit-any
+            contents: Buffer.from('not a string') as any,
+            syntax: 'scss',
+          };
+        },
+      },
+      err => {
+        expect(() => {
+          throw err;
+        }).toThrowLegacyException({
+          line: 1,
+          includes:
+            'Invalid argument (contents): must be a string but was: ' +
+            "Buffer: Instance of 'NativeUint8List'",
+        });
+        done();
+      }
+    );
+  });
+});

js-api-spec/utils.ts

@@ -27,8 +27,15 @@ declare global {
        *
        * If `url` is passed, asserts that the exception has a span with the
        * given URL.
+       *
+       * If `includes` is passed, asserts that the exception's `sassMessage`
+       * contains the given `includes` string.
        */
-      toThrowSassException(object?: {line?: number; url?: string | URL}): R;
+      toThrowSassException(object?: {
+        line?: number;
+        url?: string | URL;
+        includes?: string;
+      }): R;
 
       /**
        * Matches a callback that throws a `sass.Exception` with a span that has
@@ -62,6 +69,7 @@ interface ToThrowSassExceptionOptions {
   line?: number;
   url?: string | URL;
   noUrl?: boolean;
+  includes?: string;
 }
 
 interface SyncExpectationResult {
@@ -258,7 +266,7 @@ expect.extend({
  */
 function verifyThrown(
   thrown: unknown,
-  {line, url, noUrl}: ToThrowSassExceptionOptions
+  {line, url, noUrl, includes}: ToThrowSassExceptionOptions
 ): SyncExpectationResult {
   if (!(thrown instanceof sass.Exception)) {
     return {
@@ -288,6 +296,14 @@ function verifyThrown(
       message: () => `expected a sassMessage field:\n${thrown}`,
       pass: false,
     };
+  } else if (includes && !thrown.sassMessage.includes(includes)) {
+    return {
+      message: () =>
+        `expected sassMessage to contain ${includes}, was ` +
+        `${thrown.sassMessage}:\n` +
+        `${thrown}`,
+      pass: false,
+    };
   } else if (!thrown.sassStack) {
     return {
       message: () => `expected a sassStack field:\n${thrown}`,