[Shared Resources] Add JS API tests #1954
Conversation
There was a problem hiding this comment.
I added some basic tests to verify the behavior described in the proposal. I think we should also run all the tests from compile.node.test.ts and compile.test.ts. What do you think of parametrizing these tests to accept both the standalone compile* functions as well as the compiler methods?
There was a problem hiding this comment.
Actually, thinking about this more, it's probably a good idea to verify that the async compiler in particular gracefully handles multiple concurrent compilations, especially when those compilations are "chatty" (calling custom functions, importers, and loggers). Probably just one integration test case is sufficient as long as it handles a large number of chatty compilations at once and verifies that the right compilation calls get the right events.
Edit: I suppose we should also verify that at least two concurrent compilations work for the sync compiler, since it's theoretically possible to start a second one in a callback from the first.
| const result = compiler.compileString( | ||
| '@import "bar"; .fn {value: foo(baz)}', | ||
| {importers, functions, logger} | ||
| ); |
There was a problem hiding this comment.
we should also verify that at least two concurrent compilations work for the sync compiler, since it's theoretically possible to start a second one in a callback from the first.
I'm not sure I 100% understand the request, but this is my attempt to do it by using importers and functions similar to the async case
There was a problem hiding this comment.
I want to testing running multiple synchronous compilations at the same time. This means you'll need to start a second compilation within a callback for the first one.
jerivas
force-pushed
the
feature.shared-resources
branch
from
3834597 to
286e7fa
Compare
js-api-spec/compiler.test.ts
Outdated
| export const getSlowImporter = (callback: () => void) => ({ | ||
| canonicalize: async () => new URL('foo:bar'), | ||
| load: async () => { | ||
| await new Promise(resolve => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
Rather than using a timeout when testing that concurrent systems wait for a given event, it's usually better to have a promise whose resolution you control explicitly so you can guarantee it won't finish until you want it to (and then you don't end up writing tests that take a lot longer than they need to because of delays). You can run await Promise(resolve => setTimeout(resolve, 0)) to force all outstanding non-time-based tasks to complete, then resolve the promise and verify that the behavior happens as expected.
js-api-spec/compiler.test.ts
Outdated
| export const getLogger = () => ({debug: spy(() => {})}); | ||
|
|
||
| /* A slow importer that executes a callback after a delay */ | ||
| export const getSlowImporter = (callback: () => void) => ({ |
There was a problem hiding this comment.
Declare functions as export function, and don't forget to include a return type.
js-api-spec/compiler.node.test.ts
Outdated
| canonicalize: () => new URL('foo:bar'), | ||
| load: () => ({ | ||
| contents: compiler.compile(dir('input-nested.scss')).css, | ||
| syntax: 'scss' as const, |
There was a problem hiding this comment.
Nit: Rather than as const, I think it's cleaner to add a type declaration to the variable itself (const nestedImporter: Importer) so that it infers the correct types for all its components. Similarly for the top-level fields in compiler.test.ts.
js-api-spec/compiler.node.test.ts
Outdated
|
|
||
| describe('AsyncCompiler', () => { | ||
| let compiler: AsyncCompiler; | ||
| const runs = 1000; // Number of concurrent compilations to run |
There was a problem hiding this comment.
Declare this in the test where it's used
js-api-spec/compiler.test.ts
Outdated
| canonicalize: (url: string) => new URL(`u:${url}`), | ||
| load: (url: typeof URL) => ({ |
There was a problem hiding this comment.
I think now that importers itself is typed you don't need to type the parameters here (or in asyncImporters).
| expect(() => compiler.compileString('$a: b; c {d: $a}')).toThrowError(); | ||
| }); | ||
| }); | ||
| it('errors if constructor invoked directly', () => { |
* main: Use lts/* and lts/-n for node version (sass#1957)
….shared-resources
Issue
Blocked until proposal is accepted.
[skip dart-sass]
[skip sass-embedded]