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 specs for configuration through imports #1497

Merged
merged 3 commits into from Nov 26, 2019

Conversation

@jathak
Copy link
Member

jathak commented Nov 19, 2019

See sass/sass#2772

[skip dart-sass]

jathak added a commit to sass/dart-sass that referenced this pull request Nov 19, 2019
@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 19, 2019

@nex3: What additional specs should be added for this?

jathak added a commit to sass/dart-sass that referenced this pull request Nov 19, 2019
jathak added a commit to sass/dart-sass that referenced this pull request Nov 19, 2019
jathak added a commit to sass/dart-sass that referenced this pull request Nov 19, 2019
jathak added a commit to sass/dart-sass that referenced this pull request Nov 19, 2019

<===>
================================================================================
<===> same-file/input.scss

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor
Suggested change
<===> same-file/input.scss
<===> same_file/input.scss

https://github.com/sass/sass-spec/blob/master/STYLE_GUIDE.md#prefer-underscore-separated-paths


<===>
================================================================================
<===> same-file/input.scss

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

We try to keep each test focused on only testing one thing at a time rather than explicitly testing real-world use cases. This one's testing three things:

  • Setting a configuration variable before an import in the same file triggers configuration behavior.
  • Configuration behavior works through an as clause.
  • Configuration behavior works through a .import file.

These make it harder to figure out what's specific to one test and thereby whether that test needs to be deleted or updated when refactoring. They also mean that if an implementation breaks any of the three behaviors, this test will fail, which makes it harder to narrow down bugs. I suggest writing this test as:

<===> same_file/input.scss
$a: configured;
@import "midstream";

<===> same-file/_midstream.scss
@forward "upstream";

<===> same-file/_upstream.scss
$a: original !default;
b {c: $a}

and adding a separate test dedicated to testing what happens with as (I don't think you necessarily need to test the interaction with .import).


<===>
================================================================================
<===> import-twice/with-change/input.scss

This comment has been minimized.

Copy link
@nex3

nex3 Nov 19, 2019

Contributor

I think you also want a test that verifies that, after the second import, the variable does have the new value even though the module hasn't been reloaded.

@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Nov 19, 2019

It just occurred to me that you should probably also add a spec for a local variable set for a nested import.

@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Nov 19, 2019

Also, add a spec that verifies no error is thrown if a variable is set in the import context that isn't defined in the forwarded module.

@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Nov 19, 2019

And based on our conversation in the implementation CL:

  • A spec that verifies that, if the midstream file overrides the input's definition of the variable, the input's definition is still used in the configuration.
  • A spec that verifies that, if the midstream file defines a new variable with the same name as a configurable variable, it doesn't get included in the config.
@jathak jathak force-pushed the import-configuration branch from 52394c1 to 1325d44 Nov 20, 2019
@jathak

This comment has been minimized.

Copy link
Member Author

jathak commented Nov 20, 2019

Okay, I think I added all the tests you suggested. The one for confirming that a declaration between duplicate @import rules still sets it in the current file is todo for now because of sass/dart-sass#888.

@nex3
nex3 approved these changes Nov 20, 2019
Copy link
Contributor

nex3 left a comment

Very nice!

@jathak jathak force-pushed the import-configuration branch from 1325d44 to f9d7ad8 Nov 25, 2019
jathak added a commit to sass/dart-sass that referenced this pull request Nov 25, 2019
jathak added a commit to sass/dart-sass that referenced this pull request Nov 26, 2019
@jathak jathak merged commit 0749da4 into master Nov 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jathak jathak deleted the import-configuration branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.