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.

Sign up for GitHub

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

Jump to bottom

test(internal): Accommodate property-based testing attempts to delete an Array's length #11067

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

test(internal): Accommodate property-based testing attempts to delete an Array's length #11067

wants to merge 7 commits into from
+331 −208

Conversation

gibson042
Copy link
Member

Ref #10807

@gibson042 gibson042 requested a review from a team as a code owner February 28, 2025 04:56
@cloudflare-workers-and-pages Cloudflare Workers and Pages
Copy link

cloudflare-workers-and-pages bot commented Feb 28, 2025 •
edited
Loading

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1bbd593
Status: ✅  Deploy successful!
Preview URL: https://ac163cf3.agoric-sdk.pages.dev
Branch Preview URL: https://gibson-10807-followup.agoric-sdk.pages.dev

View logs

mhofman
mhofman reviewed Feb 28, 2025
View reviewed changes
Copy link
Member

@mhofman mhofman 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 not sure I follow what the test is trying to do, but if the problem is explicitly with length, why not special case it?

@gibson042
Copy link
Member Author

I'm not sure I follow what the test is trying to do, but if the problem is explicitly with length, why not special case it?

That ended up being less reasonable, and would also be more brittle (e.g., the prototype property of a function or class is similarly non-configurable).

michaelfig
michaelfig approved these changes Feb 28, 2025
View reviewed changes
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM! Just one question below that I'd like you to address.

@gibson042 gibson042 added the automerge:rebase Automatically rebase updates, then merge label Feb 28, 2025
@gibson042 gibson042 force-pushed the gibson-10807-followup branch from dbd604d to 1bbd593 Compare February 28, 2025 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhofman mhofman mhofman left review comments

@michaelfig michaelfig michaelfig approved these changes

Assignees
No one assigned
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gibson042 @mhofman @michaelfig