add tests for first class mixins by connorskees · Pull Request #1933 · sass/sass-spec

connorskees · 2023-08-25T04:03:02Z

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

connorskees · 2023-08-26T04:24:13Z

It seems like, from some comments I read, that these tests are typically auto-generated? Is there documentation somewhere on how to generate these?

nex3 · 2023-08-30T00:24:47Z

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:

<===> test/input.scss
a {b: }

<===> test/error

then run npm run sass-spec --interactive. When it gets to one of these test cases, use O! to tell it to overwrite the output for all tests that have similar failures (in this case, "the error doesn't match the expected error output") to match the actual output generated by the test runner.

For bonus points, explicitly document this pattern in the README 🙂.

…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

previous failure seems spurious? at least, it says the error lies with npm

nex3 · 2023-09-21T22:08:58Z

spec/core_functions/meta/accepts_content.hrx

+@use "sass:meta";
+
+@mixin foo() {
+    @content;


Use two spaces for indentation

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.

spec/core_functions/meta/accepts_content.hrx

spec/core_functions/meta/apply.hrx

spec/core_functions/meta/get_mixin/scope.hrx

spec/core_functions/meta/module_mixins.hrx

nex3

Almost there!

spec/core_functions/meta/accepts_content.hrx

spec/core_functions/meta/apply.hrx

spec/core_functions/meta/get_mixin/content.hrx

spec/core_functions/meta/apply.hrx

spec/core_functions/meta/get_mixin/content.hrx

…rst-class-mixins

ntkme · 2023-09-29T17:30:33Z

Can we add a js-api test can round-trip a mixin reference from Sass similar to this:

sass-spec/js-api-spec/value/function.test.ts

Lines 9 to 31 in dff3578

    
           it('can round-trip a function reference from Sass', () => { 
        
             const fn = spy(args => { 
        
               expect(args).toBeArrayOfSize(1); 
        
               expect(args[0]).toBeInstanceOf(SassFunction); 
        
               return args[0]; 
        
             }); 
        
             expect( 
        
               compileString( 
        
                 ` 
        
                 @use 'sass:meta'; 
        
                 @function plusOne($n) {@return $n + 1} 
        
                 a {b: meta.call(foo(meta.get-function('plusOne')), 2)} 
        
               `, 
        
                 { 
        
                   functions: {'foo($arg)': fn}, 
        
                 } 
        
               ).css 
        
             ).toBe('a {\n  b: 3;\n}'); 
        
             expect(fn).toHaveBeenCalled(); 
        
           });

connorskees · 2023-09-29T18:57:15Z

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.

ntkme · 2023-09-29T19:01:01Z

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 plusOne in sass code, pass it to the host via a host function foo and then the host function foo return the compiler function plusOne back to compiler, which then get invoked.

js-api-spec/value/mixin.test.ts

test ci with more prs linked in description

ntkme · 2023-09-29T19:19:57Z

You should add [skip sass-embedded] to this PR's description, and then trigger test from dart-sass or embedded-host-node repo if needed.

ntkme · 2023-09-29T19:21:57Z

Existing sass values has some test regarding assert like:

sass-spec/js-api-spec/value/argument-list.test.ts

Lines 98 to 107 in dff3578

    
           it("isn't any other type", () => { 
        
             expect(() => list.assertBoolean()).toThrow(); 
        
             expect(() => list.assertCalculation()).toThrow(); 
        
             expect(() => list.assertColor()).toThrow(); 
        
             expect(() => list.assertFunction()).toThrow(); 
        
             expect(() => list.assertMap()).toThrow(); 
        
             expect(list.tryMap()).toBe(null); 
        
             expect(() => list.assertNumber()).toThrow(); 
        
             expect(() => list.assertString()).toThrow(); 
        
           });

sass-spec/js-api-spec/value/boolean.test.ts

Lines 27 to 36 in dff3578

    
             it("isn't any other type", () => { 
        
               expect(value.assertCalculation).toThrow(); 
        
               expect(value.assertColor).toThrow(); 
        
               expect(value.assertFunction).toThrow(); 
        
               expect(value.assertMap).toThrow(); 
        
               expect(value.tryMap()).toBe(null); 
        
               expect(value.assertNumber).toThrow(); 
        
               expect(value.assertString).toThrow(); 
        
             }); 
        
           });

We should add assertMixin to these tests.

js-api-spec/value/boolean.test.ts

this is not true in browser environments, for some reason

nex3 · 2023-10-04T00:27:29Z

What was wrong with the assertion in 55fe148?

connorskees · 2023-10-04T00:31:18Z

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.

connorskees · 2023-10-04T00:44:57Z

It seems to pass with the assertion removed, so it does correctly execute assertMixin. It just isn't an instanceof SassMixin in the browser? I'm not familiar enough with the end-to-end process here to know what would be causing this and why it only fails in the browser environment. My assumption at least is that it's not a bug.

nex3 · 2023-10-04T20:34:07Z

Every Sass value object is expected to be instanceof its type—this is part of the API contract we provide to the user, so if it's not passing somewhere that's a bug that needs to be fixed.

nex3 · 2023-10-04T23:43:14Z

We use isInstanceOf() everywhere else, why wouldn't it work here?

connorskees · 2023-10-04T23:48:34Z

In this particular case, I changed the check from toBeInstanceOf to a bare instanceof check to get a better sense of the issue from the CI run. I would like for the check to eventually be toBeInstanceOf. I meant proper in the commit message in the sense of "instanceof proper".

I was hoping to be able to debug without having to download the chrome executable, but I suppose I will have to.

connorskees mentioned this pull request Aug 25, 2023

implement first class mixins sass/dart-sass#2073

Merged

connorskees added 4 commits September 8, 2023 06:54

add tests for first class mixins

a03b8f7

add more apply and get-mixin tests

99ebbf1

add another content scope test

fe8b737

tidy, add tests

634d71a

connorskees force-pushed the feat/first-class-mixins branch from c158cb4 to 634d71a Compare September 8, 2023 08:38

connorskees marked this pull request as ready for review September 8, 2023 08:38

connorskees added 11 commits September 8, 2023 13:17

resolve lints

3168534

fix formatting

cb3b2d4

ignore libsass

1dcde0f

formatting

7058169

add apply built test, update error spans

6dfcf9f

Merge branch 'main' of https://github.com/sass/sass-spec into feat/fi…

68d906b

…rst-class-mixins

lint spec

f1dc81d

Merge branch 'main' of https://github.com/sass/sass-spec into feat/fi…

9384c56

…rst-class-mixins

Merge branch 'main' of https://github.com/sass/sass-spec into feat/fi…

e6d89d9

…rst-class-mixins

bump ci

a6874bf

previous failure seems spurious? at least, it says the error lies with npm

nex3 requested changes Sep 21, 2023

View reviewed changes

connorskees added 6 commits September 22, 2023 18:04

pr review

a203dfc

more pr review

565122a

fix lint

bdf1d4f

ignore libsass

ff5560f

add apply tests

2617471

make more things be on a single line

ad746a1

connorskees requested a review from nex3 September 22, 2023 19:14

nex3 requested changes Sep 22, 2023

View reviewed changes

pr review

ca84c9c

nex3 requested changes Sep 27, 2023

View reviewed changes

spec/core_functions/meta/apply.hrx Show resolved Hide resolved

spec/core_functions/meta/get_mixin/content.hrx Outdated Show resolved Hide resolved

add different rest tests, move error tests

6f0fc96

connorskees requested a review from nex3 September 27, 2023 23:41

nex3 approved these changes Sep 27, 2023

View reviewed changes

connorskees mentioned this pull request Sep 28, 2023

Dynamic mixin invocation sass/sass#626

Closed

7 tasks

Merge branch 'main' of https://github.com/sass/sass-spec into feat/fi…

156da5b

…rst-class-mixins

connorskees mentioned this pull request Sep 29, 2023

add first class mixins sass/embedded-host-node#253

Merged

add js api mixin test

f918847

ntkme reviewed Sep 29, 2023

View reviewed changes

js-api-spec/value/mixin.test.ts Outdated Show resolved Hide resolved

connorskees added 2 commits September 29, 2023 19:12

use apply in test

36e7d78

bump ci

896ccf6

test ci with more prs linked in description

add negative assertions for mixin

7ecf824

ntkme reviewed Sep 29, 2023

View reviewed changes

js-api-spec/value/boolean.test.ts Outdated Show resolved Hide resolved

connorskees added 2 commits September 29, 2023 19:48

alphabetize

2b017e3

remove faulty assertion

55fe148

this is not true in browser environments, for some reason

add back instanceof assertion

d940acb

use proper instanceof check

b3131b8

stricter assertMixin, use toBeInstanceOf

7b989f0

nex3 merged commit c55ca91 into sass:main Oct 5, 2023

Conversation

connorskees commented Aug 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

connorskees commented Aug 26, 2023

Uh oh!

nex3 commented Aug 30, 2023

Uh oh!

nex3 Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

connorskees Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nex3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ntkme commented Sep 29, 2023

Uh oh!

connorskees commented Sep 29, 2023

Uh oh!

ntkme commented Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ntkme commented Sep 29, 2023

Uh oh!

ntkme commented Sep 29, 2023

Uh oh!

Uh oh!

nex3 commented Oct 4, 2023

Uh oh!

connorskees commented Oct 4, 2023

Uh oh!

connorskees commented Oct 4, 2023

Uh oh!

nex3 commented Oct 4, 2023

Uh oh!

nex3 commented Oct 4, 2023

Uh oh!

connorskees commented Oct 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

connorskees commented Aug 25, 2023 •

edited

Loading

ntkme commented Sep 29, 2023 •

edited

Loading