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

Update latest sobek: bigInt, etc #3913

Merged
merged 2 commits into from
Aug 28, 2024
Merged

Update latest sobek: bigInt, etc #3913

merged 2 commits into from
Aug 28, 2024

Conversation

olegbespalov
Copy link
Contributor

@olegbespalov olegbespalov commented Aug 23, 2024

What?

  • Updates sobek to the latest version (bigInt, etc)
  • It also updates the tc39 test, bringing a new version of them and synchronise the list of the blocked list features with the grafana/sobek

This PR contains a possible breaking change since now sobek could automatically convert the golang's math's bigInt into the bigint whenever previously it was another structure

Why?

The sobek has more features, which we would like to bring to k6

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

@olegbespalov olegbespalov added the dependencies Pull requests that update a dependency file label Aug 23, 2024
@olegbespalov olegbespalov added this to the v0.54.0 milestone Aug 23, 2024
@olegbespalov olegbespalov self-assigned this Aug 23, 2024
@olegbespalov olegbespalov requested a review from a team as a code owner August 23, 2024 15:05
@olegbespalov olegbespalov requested review from mstoykov and codebien and removed request for a team August 23, 2024 15:05
@olegbespalov
Copy link
Contributor Author

Okay, it's not so straightforward:sweat_smile: I will look at the tc39 tests and adjust them accordingly. +1: For now covering PR to a draft

@olegbespalov olegbespalov marked this pull request as draft August 23, 2024 15:12
@olegbespalov olegbespalov added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Aug 28, 2024
if (value.join(":") !== expected.join(":")) {
throw new Error("Bad RSA public key modulus: " + value.join(":"));
var value = cert.publicKey ? cert.publicKey.key.n.toString(16) : null;
var expected = "dff9ea47b4241c3e548db1763502af2da7599bd8675620d82a5c547db766d928ff8126cbaf62d193976afa0c7bec872d50504cfd948d0d244c2fb63a6499720d8d600c6d7e4a4477580f346b5075bea3e6781a2c8bf5debcd2ed7b535b62a0dae5f4801f188b7d19b86cb41795882f8ca8339f2341960733868abce1695c095b488546c38369b8bfc6b182bf4606259ffff46dee808a00c929574357a57604fdd11fb1010174400e1113d906754b18aeec818673f5e2cbde1171c5d78af163a9444df32474458f963cf54bc81ecf8d8b88237db8ffae1a069438fd7cf99ef975037e7c591b60d100cf98e9a65107fd9ba5601d481cac186846e5c96bc8a9dc23";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note for the reviewers: it's the hex string representation of the bytes from before

@@ -189,13 +189,6 @@ func TestNewBundle(t *testing.T) {
"InvalidCompat", "es1", `export default function() {};`,
`invalid compatibility mode "es1". Use: "extended", "base", "experimental_enhanced"`,
},
// BigInt is not supported
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for reviewers: I dropped the case completely since I am unsure how useful it is. If reviewers find this relevant, we can come up with another not-implemented feature that could be used till the next time

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it is as useful now that basically compatibility mode is kind of defunct, so 👍

In theory we can replace it with some other functionality that isn't supported ... but 🤷

@olegbespalov
Copy link
Contributor Author

Okay, it's ready for review, but I feel we need to try test this on the webcrypto's, since potentially it could also be affected

@olegbespalov olegbespalov marked this pull request as ready for review August 28, 2024 09:29
@olegbespalov
Copy link
Contributor Author

Okay, webcrypto set of tests also look good.

image

@olegbespalov olegbespalov merged commit 8aa1aca into master Aug 28, 2024
23 checks passed
@olegbespalov olegbespalov deleted the dep/sobek branch August 28, 2024 12:19
@olegbespalov olegbespalov added the documentation-needed A PR which will need a separate PR for documentation label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change for PRs that need to be mentioned in the breaking changes section of the release notes dependencies Pull requests that update a dependency file documentation-needed A PR which will need a separate PR for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants