Skip to content

Variation of ses-ava test with deep stacks #2582

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

Closed
wants to merge 1 commit into from

Conversation

erights
Copy link
Member

@erights erights commented Mar 5, 2021

Modeled on https://github.com/endojs/endo/blob/master/packages/ses-ava/test/test-ses-ava-reject.js but with E.when rather than .then in order to show deep stacks.

Until endojs/endo#579 is fixed, both comment out the code being tested because when it is not commented out it causes the ava failure that is the point of the test, but this means that yarn test fails and therefore that CI fails.

@erights erights requested review from kriskowal and dckc March 5, 2021 05:55
@erights erights self-assigned this Mar 5, 2021
@erights erights force-pushed the ses-ava-deep-stacks branch from 935af75 to 54d2a35 Compare March 15, 2021 05:05
@erights erights changed the title WIP Variation of ses-ava test with deep stacks Variation of ses-ava test with deep stacks Mar 15, 2021
@erights erights marked this pull request as ready for review March 15, 2021 05:11
@erights
Copy link
Member Author

erights commented Mar 15, 2021

This is now ready for review.

Comment on lines +14 to +24
// Uncomment this to see something like the text in the extended comment below

/*
return E.when(Promise.resolve(null), v1 =>
E.when(v1, v2 =>
E.when(v2, _ => {
assert.typeof(88, 'string', assert.details`msg ${'NOTICE ME'}`);
}),
),
);
*/
Copy link
Member

Choose a reason for hiding this comment

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

Commented out tests seem really awkward.

Perhaps use t.skip?

Or perhaps the pattern from test-deep-stacks.js?

return q.catch(reason => {
t.assert(reason instanceof Error);
console.log('expected failure', reason);
});

I played around with it and came up with...

Suggested change
// Uncomment this to see something like the text in the extended comment below
/*
return E.when(Promise.resolve(null), v1 =>
E.when(v1, v2 =>
E.when(v2, _ => {
assert.typeof(88, 'string', assert.details`msg ${'NOTICE ME'}`);
}),
),
);
*/
const q = E.when(Promise.resolve(null), v1 =>
E.when(v1, v2 =>
E.when(v2, _ => {
assert.typeof(88, 'string', assert.details`msg ${'NOTICE ME'}`);
}),
),
);
return q.catch(reason => {
t.assert(reason instanceof Error);
t.notRegex(reason.message, /NOTICE ME/, 'message detail hidden');
console.log('expected failure', reason);
});

Copy link
Member

Choose a reason for hiding this comment

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

meanwhile our tests aren't supposed to console.log. t.log would be silent unless ava -v were used...

    t.log('expected failure', reason);

but it's not integrated with the SES console, so we get the boring...

  ✔ ses-ava-reject-deep-stacks › ses-ava reject console output
    ℹ expected failure TypeError {
        message: 'msg (a string)',
      }

I'm raising an issue on ses-ava about t.log support.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not understand this. Let's discuss soon.

@erights erights force-pushed the ses-ava-deep-stacks branch from bae5b0d to bf407ae Compare March 16, 2021 04:29
@@ -28,6 +28,7 @@
"devDependencies": {
"@agoric/assert": "^0.2.2",
"@agoric/install-ses": "^0.5.2",
"@agoric/ses-ava": "^0.1.0+1-dev",
Copy link
Member

Choose a reason for hiding this comment

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

ses-ava is coming from npm, which does not carry our dev versions.

Suggested change
"@agoric/ses-ava": "^0.1.0+1-dev",
"@agoric/ses-ava": "^0.1.0",

@erights
Copy link
Member Author

erights commented May 20, 2023

Closing in favor of endojs/endo#1591

@erights erights closed this May 20, 2023
@erights
Copy link
Member Author

erights commented May 20, 2023

Closed the wrong PR. Reopening

@erights
Copy link
Member Author

erights commented May 20, 2023

Closing in favor of endojs/endo#1592

@erights erights closed this May 20, 2023
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