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 JSC/Bun test suite #2348

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add JSC/Bun test suite #2348

wants to merge 4 commits into from

Conversation

jonluca
Copy link

@jonluca jonluca commented Jan 31, 2025

This adds JSC as a test suite, to catch issues that would appear in Safari or Bun. It also fixes a minor issue with missing type imports.

@jonluca jonluca mentioned this pull request Jan 31, 2025
2 tasks
@coveralls
Copy link

Coverage Status

coverage: 97.005%. remained the same
when pulling 9618218 on jonluca:master
into 446ca7f on Borewit:master.

Copy link
Owner

@Borewit Borewit left a comment

Choose a reason for hiding this comment

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

This is great, I did not realize I could test the Safari engine this way. I want to apply this even further, and do Bum/JSC testing in the dependencies (peek-readable and strtok3) as well.

Please avoid using your default branch for PRs, that limits options for syncing your work and pushing corrections from my end.

I added some small comments, to avoid mixing different changes.

Copy link
Owner

Choose a reason for hiding this comment

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

Please don't upgrade dependencies with this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

Good catch, but please put this fix in a separate PR

@Borewit
Copy link
Owner

Borewit commented Feb 2, 2025

@jonluca, I think we overlooked a major issue, unlike in the Apple Safari browser, Bum does actually implement the BYOB stream reader.

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