search

endojs / endo

Endo is a distributed secure JavaScript sandbox, based on SES
Apache License 2.0
829 stars 72 forks source link

Test of cleanup removal messages used a golden that is too fragile. #1973

Closed erights closed 9 months ago

erights commented 9 months ago

Describe the bug

https://github.com/endojs/endo/pull/1942 made a change to how SES init-time warnings are output to the console. This part remains AFAIK correct.

But to test it, #1942 at https://github.com/endojs/endo/blob/1ff3c4b45a4f9f6bf723d3200548db37df46989f/packages/ses/test/error/test-permit-removal-warnings.js#L23 tests the console log against a golden. This is fine when run on a platform without extra properties that would cause more such warnings to be produced. But when testing https://github.com/endojs/endo/pull/1969 on Node v22.0.0-v8-canary2024011602c728ac7a from https://nodejs.org/download/v8-canary/v22.0.0-v8-canary2024011602c728ac7a/node-v22.0.0-v8-canary2024011602c728ac7a.pkg the "extra" properties from the set-methods proposal cause test-permit-removal-warnings.js

Switching from #1969 to #1970 , which includes #1969 but always permits these new set methods, test-permit-removal-warnings.js runs fine again. This is a bug of test-permit-removal-warnings.js , because it should be tolerant of additional such properties generating such warnings.

Steps to reproduce

Install Node v22.0.0-v8-canary2024011602c728ac7a (or likely any Node 22 though I haven't tested any others)

$ git checkout markm-some-properties-already-override-protected
$ cd packages/ses
$ yarn test test/error/test-permit-removal-warnings.js

See the screenshot below for the current buggy behavior.

Expected behavior

Doing instead on the same Node 22:

$ git checkout markm-anticipate-set-methods
$ cd packages/ses
$ yarn test test/error/test-permit-removal-warnings.js

produces the expected

image

as does running on current endo master with Node 21 or earlier.

Platform environment

Node v22.0.0-v8-canary2024011602c728ac7a from https://nodejs.org/download/v8-canary/v22.0.0-v8-canary2024011602c728ac7a/node-v22.0.0-v8-canary2024011602c728ac7a.pkg

Current state of endo fork markm-some-properties-already-override-protected from current state of PR #1969

@endo/bundle-source@3.0.1-85-g6357a8e92

Not likely to be relevant, but macOS Sonoma Version 14.2.1 (23C71)

Additional context

I expect to fix PR #1969 merely by changing the relevant test( to test.skip(, masking this bug, and then to fix this bug in a separate PR. This makes it easy to reproduce this bug even after I fix #1969 .

Instead, I added that .skip in a separate PR https://github.com/endojs/endo/pull/1974 .

Screenshots

image