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

test(ses-ava): use causal console #2109

Merged
merged 10 commits into from
Mar 9, 2024
Merged

test(ses-ava): use causal console #2109

merged 10 commits into from
Mar 9, 2024

Conversation

erights
Copy link
Contributor

@erights erights commented Mar 1, 2024

closes: #611
closes: #891
refs: Agoric/agoric-sdk#8965 (comment) #647 #1467 #2107 Agoric/agoric-sdk#8662 #1798 #2101 #2107 https://github.com/Agoric/agoric-sdk/wiki/agoric-sdk-unit-testing#patterns

Description

Replaces #2101 and #2107 , though unclosed conversations there probably apply here as well.

Alter ses-ava so that

  • The exposed virtual t.log first goes through our causal console logic, to show redacted information including nesting errors, but still ultimately directs the output to the original t.log so it retains all the ordering and verbosity switching associated with Ava's original t.log.
  • When a test fails by throwing or rejecting, the error is also logged with the virtual t.log, so you see its redacted info as well, while staying associated with the error.

To get access to the SES's causal console logic, this PR also adds a new export for a privileged power, which ses-ava needs and imports using

import { makeCausalConsoleFromLogger } from 'ses/console-tools.js';

#2107 review comments raise the issue of whether such special powers should be "exported" from ses and "imported" by privileged clients vs import, as this PR currently does, or as a global binding of the start compartment. Unlike #2101 and #2107, this PR currently bundles the ses changes and the ses-ava changes together so we can coordinate changing our minds on this. Reviewers, once that settles, we can split this up into two PRs again if you wish.

Security Considerations

As of this PR, ses-ava will import makeCausalConsoleFromLogger from console-tools.js. We must ensure that this arrangement does not accidentally make these powers available to non-privileged code executing in constructed (non-start) compartments.

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

The whole point. Since this PR changes only the logging of errors or rejections from the t => {...} function, it will not by itself clean up console output in general. But at least it will cause these error reports to appear at the "right" place in the overall console output.

In addition, the "export" of makeCausalConsoleFromLogger from ses/console-tools.js enables other testing tools to build on our causal console the way ses-ava does.

Compatibility Considerations

Mostly none.

If there are programs post-processing our voluminous console output, they might need to change. However,

  • I am not aware of any such programs
  • the console output was never stable anyway, so any such programs should be adaptable enough to tolerate this change as well.

Upgrade Considerations

Nothing breaking.

  • Includes *BREAKING*: in the commit message with migration instructions for any breaking change.
  • Updates NEWS.md for user-facing changes.

@erights
Copy link
Contributor Author

erights commented Mar 1, 2024

May want to redo #701 (comment) as part of this PR. It would be a natural fit.

@erights erights requested a review from turadg March 4, 2024 16:48
Copy link
Contributor

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

Very nice! But I wonder if we can thread the unsealing/unredaction power through a bit more directly (see below).

packages/eventual-send/test/test-deep-send-ses-ava.js Outdated Show resolved Hide resolved
packages/eventual-send/test/test-deep-stacks-ses-ava.js Outdated Show resolved Hide resolved
packages/eventual-send/test/test-deep-stacks-ses-ava.js Outdated Show resolved Hide resolved
packages/ses/console-tools.js Outdated Show resolved Hide resolved
packages/ses-ava/src/ses-ava-test.js Outdated Show resolved Hide resolved
packages/ses-ava/src/ses-ava-test.js Show resolved Hide resolved
packages/ses-ava/test/test-raw-ava-reject.js Outdated Show resolved Hide resolved
packages/ses-ava/test/test-ses-ava-reject.js Outdated Show resolved Hide resolved
packages/ses/src/error/console.js Outdated Show resolved Hide resolved
packages/ses/src/error/console.js Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-ses-ava-reform branch from 1374a75 to a24136d Compare March 5, 2024 23:59
@erights
Copy link
Contributor Author

erights commented Mar 6, 2024

Apologies to my reviewers, but in responding to @kriskowal 's request about how ses makes the special console-making-power available to @endo/ses-ava at this time, I went about making the change in a way that squashed. I hope this does not make further review too difficult. I have not yet responded to the reviewer comments still open above, and will do so from this baseline.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

I’m satisfied by this coupling at a global symbol between ses and ses-ava since this is a stopgap on a path and not where we want to land for error unsealing in general.

I defer to @gibson042 for the final stamp since he has pending feedback!

packages/ses/index.js Show resolved Hide resolved
packages/ses-ava/src/ses-ava-test.js Show resolved Hide resolved
packages/ses-ava/src/ses-ava-test.js Outdated Show resolved Hide resolved
packages/ses-ava/src/ses-ava-test.js Outdated Show resolved Hide resolved
packages/ses/src/console-shim.js Outdated Show resolved Hide resolved
Copy link
Contributor

@dckc dckc left a comment

Choose a reason for hiding this comment

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

I gather @gibson042 and @kriskowal have this review well in hand.

@turadg turadg removed their request for review March 6, 2024 21:50
@erights erights force-pushed the markm-ses-ava-reform branch 2 times, most recently from 8cd0a8d to 2b4f0ce Compare March 8, 2024 20:48
@erights erights force-pushed the markm-ses-ava-reform branch from 7823bbe to c10e187 Compare March 9, 2024 01:46
@erights erights enabled auto-merge (squash) March 9, 2024 01:48
@erights erights disabled auto-merge March 9, 2024 01:54
@erights
Copy link
Contributor Author

erights commented Mar 9, 2024

Ping. I addressed all reviewer suggestions. PTAL. thanks.

@erights erights requested review from kriskowal and gibson042 March 9, 2024 01:59
@erights erights merged commit 0a5c555 into master Mar 9, 2024
14 checks passed
@erights erights deleted the markm-ses-ava-reform branch March 9, 2024 02:24
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.

Ses-ava console virtualization: report to t.log instead of console ses-ava support for t.log
5 participants