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

Don't log JS exception details by default #6205

Merged

Conversation

eddyashton
Copy link
Member

Restoring intended initial values for these 2 bools, which were accidentally updated to verbose-by-default in a recent refactor.

@eddyashton eddyashton requested a review from a team as a code owner May 24, 2024 12:33
@achamayou
Copy link
Member

@eddyashton that seems like a thing we really ought to have a test for.

@eddyashton
Copy link
Member Author

eddyashton commented May 24, 2024

@eddyashton that seems like a thing we really ought to have a test for.

I 100% agree, but while trying to add that I've decided it deserves a bigger cleanup. The entire way we fetch and report the JS config is messy, with various paths taken to determine defaults when the JSEngine KV value doesn't exist. I think we should squash that entirely - the default constructor of a JSEngine contains the default values, you get the default if there's none in the KV, and then you always have a JSEngine object to work with. I'll do that, but not in this PR.

@eddyashton eddyashton merged commit bdac0c4 into microsoft:main May 24, 2024
21 checks passed
@achamayou
Copy link
Member

@eddyashton I agree with that too, but I think in this case it's just a canary that runs an endpoint that produces an error and checks nothing gets logged unless this is on, it's quite orthogonal, and a good thing to have around as we refactor the JS runtime substantially.

@achamayou
Copy link
Member

@eddyashton So we have (almost!) exactly that test:

def test_js_exception_output(network, args):

But! Because it doesn't run first, and set_js_runtime_options is used in a number of other tests before, a change that affects the default will not be caught.

I will move it up.

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.

2 participants