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 support for running JS Spec Tests in browser context. #1902

Merged
merged 35 commits into from
May 19, 2023

Conversation

jgerigmeyer
Copy link
Contributor

@jgerigmeyer jgerigmeyer commented Apr 21, 2023

TODO:

[skip dart-sass]

Copy link
Contributor Author

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 This is still WIP, but it'd be good to have your initial review on the overall approach here, and how the tests and setup files are re-arranged a bit.

Comment on lines 172 to 173
# TODO update this to use https://github.com/sass/dart-sass.git
git clone https://github.com/oddbird/dart-sass.git ../dart-sass --depth 1 --branch browser
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before merging, this should be updated:

Suggested change
# TODO update this to use https://github.com/sass/dart-sass.git
git clone https://github.com/oddbird/dart-sass.git ../dart-sass --depth 1 --branch browser
git clone https://github.com/sass/dart-sass.git ../dart-sass --depth 1

js-api-spec.ts Show resolved Hide resolved
Copy link
Contributor

@nex3 nex3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks good to me!

@jgerigmeyer jgerigmeyer requested a review from nex3 April 26, 2023 20:29
@jgerigmeyer
Copy link
Contributor Author

jgerigmeyer commented Apr 26, 2023

@nex3 Looking at the remainder of the JS API spec tests, I'd like your feedback on which ones we should be supporting in a browser context. Here are my initial thoughts/questions:

  • legacy/ -- I assume we can skip these
  • value/ -- Do we want to run these? If so, we'll need to expose the various types (e.g. SassArgumentList, SassList, sassNull, etc) for the browser build.
  • function.test.ts -- Similar to value/; if we want to run these, we'll need to expose CustomFunction, SassString, and sassNull.
  • importer.test.ts -- Do we want to run (some of) these? I assume we'd skip the ones that rely on FS access (via sandbox()), but it's possible we could run some of the tests.
  • logger.test.ts -- I assume we want some logging/debug support in the browser build, but given the differences between console and stdio, it might make sense to write a subset of logging tests specifically for the browser context, and not copy any of these over to run in both contexts?

@nex3
Copy link
Contributor

nex3 commented Apr 27, 2023

@nex3 Looking at the remainder of the JS API spec tests, I'd like your feedback on which ones we should be supporting in a browser context. Here are my initial thoughts/questions:

  • legacy/ -- I assume we can skip these

Yep, totally ignore these.

  • value/ -- Do we want to run these? If so, we'll need to expose the various types (e.g. SassArgumentList, SassList, sassNull, etc) for the browser build.
  • function.test.ts -- Similar to value/; if we want to run these, we'll need to expose CustomFunction, SassString, and sassNull.

We should definitely support these. We want people to be able to use custom functions on the browser.

  • importer.test.ts -- Do we want to run (some of) these? I assume we'd skip the ones that rely on FS access (via sandbox()), but it's possible we could run some of the tests.

Yeah, I think we should separate out the ones that test importer/filesystem interactions into a separate file and run the rest on the browser.

  • logger.test.ts -- I assume we want some logging/debug support in the browser build, but given the differences between console and stdio, it might make sense to right a subset of logging tests specifically for the browser context, and not copy any of these over to run in both contexts?

We could potentially abstract captureStdio() into a function that captures stdio on Node and console.log() on the browser. If that's infeasible, then maybe we should have three test files under a logger/ directory: Node-specific, browser-specific, and platform-agnostic.

@jgerigmeyer jgerigmeyer marked this pull request as ready for review May 2, 2023 16:28
Copy link
Contributor Author

@jgerigmeyer jgerigmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nex3 @jerivas I think this is ready for another (close to final?) round of review!

js-api-spec/setup.ts Outdated Show resolved Hide resolved
karma.config.js Show resolved Hide resolved
Copy link
Contributor

@jerivas jerivas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jgerigmeyer

This comment was marked as resolved.

@jgerigmeyer
Copy link
Contributor Author

@jgerigmeyer jgerigmeyer changed the title WIP: Add support for running JS Spec Tests in browser context. Add support for running JS Spec Tests in browser context. May 16, 2023
jgerigmeyer and others added 3 commits May 16, 2023 17:15
* main:
  Build the embedded protocol before running Dart Sass (sass#1908)
This PR is now linked from the Dart Sass PR, so we can use that to
verify that the tests are working.
js-api-spec/compile.browser.test.ts Outdated Show resolved Hide resolved
js-api-spec/setup.ts Outdated Show resolved Hide resolved
karma.config.js Show resolved Hide resolved
@jgerigmeyer jgerigmeyer requested a review from nex3 May 17, 2023 21:40
nex3 and others added 2 commits May 18, 2023 17:39
* main:
  Update command to run js-api-spec tests (sass#1909)
@jgerigmeyer
Copy link
Contributor Author

@nex3 Full transparency, I just realized yesterday that Karma is officially deprecated as of a few weeks ago. 🤦

I think there's potential for switching to either Web Test Runner or jasmine-browser-runner, but after trying them out a bit today I ran out of steam. I'd be happy to chat more about pros/cons of those two options, but I think I'd recommend that this is a good start, and we can consider a switch to Web Test Runner sooner or later.

@nex3 nex3 merged commit f53bab1 into sass:main May 19, 2023
@jgerigmeyer jgerigmeyer deleted the browser-tests branch May 19, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants