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): option to use t.log #2101

Conversation

erights
Copy link
Contributor

@erights erights commented Feb 24, 2024

Staged on #2107

closes: #XXXX
refs: Agoric/agoric-sdk#8965 (comment) #611 #647 #891 #1467 #2107 Agoric/agoric-sdk#8662 #1798

Description

Alter ses-ava so that it ultimately uses t.log to log errors or rejections from the t => {...} function.

Security Considerations

As of this PR, ses-ava will import from console-tools.js, as exported by ses in #2107 . We must ensure that this arrangement does not accidentally make these powers available to non-privileged code executing in constructed (non-start) compartments. See "Security Considerations" at #2107

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.

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. Nothing newsworthy as of this PR by itself.

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

@erights erights self-assigned this Feb 24, 2024
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.

Nice; this is definitely an improvement!

packages/ses-ava/src/ses-ava-test.js Show resolved Hide resolved
packages/ses-ava/test/prepare-test-env-ava.js Outdated Show resolved Hide resolved
@erights erights force-pushed the markm-ses-ava-quieter-logging branch from a0ce2a3 to 2458798 Compare February 28, 2024 22:47
@erights erights changed the base branch from master to markm-export-console-tools-support February 28, 2024 22:47
@erights erights force-pushed the markm-export-console-tools-support branch from 87982e5 to 7a7ff1a Compare February 29, 2024 02:20
@erights erights force-pushed the markm-ses-ava-quieter-logging branch 2 times, most recently from caf11d6 to f1b5d40 Compare February 29, 2024 03:25
@erights erights marked this pull request as ready for review February 29, 2024 03:27
@erights
Copy link
Contributor Author

erights commented Feb 29, 2024

This is now actually ready for review.

Enough has changed since you approved that I request new approvals. (I have no idea how to clear the old ones, or even if that's possible.)

Note that this PR now depends on, and is staged on, #2107, so that would need to get reviewed and merged before this one could. Please review that one too. It should be straightforward.

@erights erights force-pushed the markm-ses-ava-quieter-logging branch 2 times, most recently from 7f8b02c to 92e34c9 Compare February 29, 2024 03:47
@erights erights force-pushed the markm-ses-ava-quieter-logging branch from 92e34c9 to 4edbbbd Compare February 29, 2024 21:38
@erights erights marked this pull request as draft March 1, 2024 00:48
@erights
Copy link
Contributor Author

erights commented Mar 1, 2024

Converting to Draft. See #2107 (comment)

@erights erights force-pushed the markm-ses-ava-quieter-logging branch from 8e3c1c6 to 37f2093 Compare March 1, 2024 05:28
@erights erights force-pushed the markm-ses-ava-quieter-logging branch from 37f2093 to 650e719 Compare March 1, 2024 05:41
@erights
Copy link
Contributor Author

erights commented Mar 1, 2024

Closing in favor of #2109

@erights erights closed this Mar 1, 2024
@erights
Copy link
Contributor Author

erights commented Mar 1, 2024

#2109 is sufficiently different that it needs a from-scratch review.

@erights erights mentioned this pull request Mar 1, 2024
2 tasks
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.

3 participants