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

Add tests for indented syntax #66

Merged
merged 3 commits into from Jul 17, 2019

Conversation

Projects
None yet
2 participants
@jathak
Copy link
Member

commented Jul 10, 2019

Resolves #5.

This adds copies of the unprefix and namespace_mixin tests that use the indented syntax and the shorthand +/= syntax for mixins.

I initially thought about making indented syntax copies of all tests, but that felt redundant, as these two tests should have enough to test the basics of the indented syntax and the one special case I could find (the mixin shorthand syntax).

Are there any other things particular to the indented syntax that would be good to add tests for or do these seem fine?

Add tests for indented syntax
Resolves #5.

This adds copies of the unprefix and namespace_mixin tests that use the
indented syntax and the shorthand `+`/`=` syntax for mixins.

@jathak jathak requested a review from nex3 Jul 10, 2019

@jathak

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Hmm. Not sure why the Node tests are failing on Travis. They pass on my machine

@nex3

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

It's probably a good idea to add comments to these tests indicating why they were chosen in particular.

Hmm. Not sure why the Node tests are failing on Travis. They pass on my machine

Are you using the same Node and Dart versions? Usually that sort of thing is caused by version skew.

@jathak

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Looks like running pub upgrade caused the Node tests to start failing on my machine, so my guess is that something between sass 1.21.0 and sass 1.22.3 broke them.

From the stack trace it looks like it's some sort of type error, but I'm not sure where exactly it's coming from. Is there an easy way to convert the JS stack trace to a Dart one with the dart2js source map?

@nex3

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

You can try to do something manually with https://pub.dev/packages/source_map_stack_trace, but I tend to find it easier to just debug into the generated JS.

@jathak

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Okay, so it seems like the error is occuring when the migrator calls FilesystemImporter.canonicalize on the entrypoint path that's passed in. Not sure why yet

@jathak

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Okay, so I looks like this cast is what's causing the exception, with the error message being:

CastError: Instance of 'NullError': type 'NullError' is not a subtype of type '_SystemError'

canonicalize is checking whether entrypoint.import.scss exists, which it doesn't, but the exception that the Node function throws isn't the expected _SystemError for some reason.

@jathak

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Relevant compiled JS:

    interceptedTypeCast: function(value, property) {
      var t1;
      if (value != null)
        t1 = (typeof value === "object" || typeof value === "function") && J.getInterceptor$(value)[property];
      else
        t1 = true;
      if (t1)
        return value;
      H.propertyTypeCastError(value, property);
    },
...
    fileExists: function(path) {
      var error, systemError, t1, exception;
      try {
        t1 = J.isFile$0$x(J.statSync$1$x(self.fs, path));
        return t1;
      } catch (exception) {
        error = H.unwrapException(exception);
        systemError = H.interceptedTypeCast(error, "$is_SystemError");
        if (J.$eq$(J.get$code$x(systemError), "ENOENT"))
          return false;
        throw exception;
      }
    },
@nex3

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2019

I assume the path that's getting passed in is null, which it shouldn't be. What's the call stack for the call to fileExists()?

@jathak

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

I figured it out. _fs is null when we try to call _fs.statSync(path).isFile().

Could this be related to sass/dart-sass#727?

@jathak

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

#67 should fix this

@nex3

nex3 approved these changes Jul 17, 2019

@jathak jathak merged commit 1302675 into master Jul 17, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@jathak jathak deleted the indented-syntax branch Jul 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.