-
Notifications
You must be signed in to change notification settings - Fork 212
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
fix(xsnap): supply missing file, line numbers based on sourceURL #3776
Conversation
685df45
to
7f84e44
Compare
@warner note: this probably enables the debugger network connection in release / production mode. |
Also: I thought about testing the interaction with Do you want a unit test involving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Bummer... a test fails in ci but passes locally.
|
7f84e44
to
796c438
Compare
@warner you asked how different... see https://gist.github.com/dckc/9513e1acc1c263ee962b0e282cf48153 |
More on that soon... |
8b65227
to
1464e42
Compare
@katelynsills was there anything more than trial-and-error to the 4.2M computron figure in I experimentally discovered that this change makes the test pass: - initial: 4_200_000n, // startInstance seems to take around this much
+ initial: 4_300_000n, // startInstance seems to take around this much @warner thought about having the test compute this number by measurement in a pre-test phase, since this test isn't so much about the absolute numbers. But we agreed that the absolute numbers are somewhat relevant and they should be tested somewhere, so that's not urgent. |
It was unfortunately trial and error.
Ah, yeah, that seems like a good idea. This test is intended to confirm that the meter refills when expected and then the vat shuts down when the meter runs out when expected. So, the pre-test data would help me know "when expected" actually is. But you're also right that we need the absolute numbers somewhere too - in other words, I'd like to know when changes to agoric-sdk will result in broken assumptions about computrons for actions. I can make an issue for this, and feel free to tag me and any related issues with a TODO comment if that's helpful. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This a review of only the Zoe changes, but those look good to me. I'll work on making the metering test more robust.
Added an issue for making the "refillMeter" test more robust: #3804 |
f9ea4ae1f1d3 Aug 27 adds sourceURL support
xs-metering-10 was based on moddable rev 6024736 so this reflects metering changes in 6024736..6d7f33f
1464e42
to
91a6f6d
Compare
fixes #2578
depends on agoric-labs/xsnap-pub#1
@michaelfig @warner I was surprised to find some metering diffs when I grabbed the
sourceURL
support from the moddable public branch. They're inTypedArray
(xsDataView.c
), which our "has metering changed?" test is insensitive to. Should I update fromxs-meter-10
to 11?As of #3607 we were at 91b0b8bfdc4c, which is part of Moddable's public branch.
But somehow in chore: publich 7.0.0 we regressed to some of our tweaks (to
6024736
).relevant diffs from
6024736
-6d7f33f
: https://gist.github.com/dckc/9513e1acc1c263ee962b0e282cf48153