-
Notifications
You must be signed in to change notification settings - Fork 89
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 first class mixins #1933
Conversation
It seems like, from some comments I read, that these tests are typically auto-generated? Is there documentation somewhere on how to generate these? |
The test cases are written by hand, but for error tests specifically the output can be auto-generated. Just write test cases with empty error output:
then run For bonus points, explicitly document this pattern in the README 🙂. |
c158cb4
to
634d71a
Compare
…rst-class-mixins
this test _shouldn't_ have a different error message as a result of the mixin changes, but i am updating it now so that CI is green, before i get a chance to investigate why this change is occurring. i have verified that the change to this test is related to the changes to mixins
…rst-class-mixins
…rst-class-mixins
@use "sass:meta"; | ||
|
||
@mixin foo() { | ||
@content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved. It would be nice if lint-spec
enforced this or if lint-spec --fix
fixed this -- it was somewhat tedious to grep for
to find all places with the wrong indentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there!
…rst-class-mixins
Can we add a js-api test sass-spec/js-api-spec/value/function.test.ts Lines 9 to 31 in dff3578
|
Do we have JS API tests today that read a value from Sass? The function tests (and all the tests I've looked at so far) seem to involve declaring a function/another value in JavaScript and sending it to Sass, which is not possible with mixins. |
The sass function test I linked above declares a sass function |
You should add |
Existing sass values has some test regarding sass-spec/js-api-spec/value/argument-list.test.ts Lines 98 to 107 in dff3578
sass-spec/js-api-spec/value/boolean.test.ts Lines 27 to 36 in dff3578
We should add |
this is not true in browser environments, for some reason
What was wrong with the assertion in 55fe148? |
I'm not sure -- I was somewhat hoping for a subsequent CI run to validate that the later checks are true to get a better idea. This test failed in https://github.com/sass/dart-sass/actions/runs/6399530354/job/17372158557, only on the browser environment. I am expecting reproducing the run in the browser environment locally to be hard. |
It seems to pass with the assertion removed, so it does correctly execute |
Every Sass value object is expected to be |
We use |
In this particular case, I changed the check from I was hoping to be able to debug without having to download the chrome executable, but I suppose I will have to. |
This is some initial work at adding the baseline tests for first class mixins.
Embedded node host PR: sass/embedded-host-node#253
Tracking issue: sass/sass#626
Protobuf changes: sass/sass#3674
dart-sass changes: sass/dart-sass#2073
[skip dart-sass]
[skip sass-embedded]
Closes #1927