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 a test for QuickJS runtime options #4371

Closed
MahatiC opened this issue Oct 18, 2022 · 6 comments
Closed

Add a test for QuickJS runtime options #4371

MahatiC opened this issue Oct 18, 2022 · 6 comments
Assignees
Milestone

Comments

@MahatiC
Copy link
Member

MahatiC commented Oct 18, 2022

As mentioned here #4344 (comment)

@MahatiC
Copy link
Member Author

MahatiC commented Oct 19, 2022

@achamayou Julien pointed out the existing test for this https://github.com/microsoft/CCF/blob/main/tests/js-custom-authorization/custom_authorization.py#L43-L69

The test values don't seem to match with the defaults set in the code though. I'm not yet sure at this point how the default values were derived.

Edit: On taking another look at it, the test values don't have to match the defaults. but still unsure on how the original defaults were derived.

@achamayou
Copy link
Member

achamayou commented Oct 19, 2022

@MahatiC the idea is to allocate a lot less (*) than the limit, check it works, and more than the limit, and check it fails.

(*) too close and it will fail, because there will be other allocations we can't precisely control etc.

Ideally this test would call an endpoint that exposes the value, or use the config directly, and compute the values as limit - 50% and limit + 5% or something like that.

@achamayou
Copy link
Member

For the stack size limit, an (large) recursive call is probably the best way to go.

@MahatiC
Copy link
Member Author

MahatiC commented Oct 19, 2022

Sure that makes sense, and I assume the current default heap size of 100 * 1024 * 1024 in the code was derived empirically?

@achamayou
Copy link
Member

Yes, I think it was our default enclave heap size for a long time.

@achamayou achamayou added this to the 4.x milestone May 15, 2023
@achamayou
Copy link
Member

Done in #5594 and #5668

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

No branches or pull requests

2 participants