Skip to content

[explicit-resource-management] Add remaining tests specific to AsyncDisposableStack#4478

Open
rbuckton wants to merge 7 commits intotc39:mainfrom
rbuckton:explicit-resource-management-AsyncDisposableStack
Open

[explicit-resource-management] Add remaining tests specific to AsyncDisposableStack#4478
rbuckton wants to merge 7 commits intotc39:mainfrom
rbuckton:explicit-resource-management-AsyncDisposableStack

Conversation

@rbuckton
Copy link
Contributor

In an effort to make review more manageable, this extracts the remaining tests specific to AsyncDisposableStack from #3866

@ptomato ptomato force-pushed the explicit-resource-management-AsyncDisposableStack branch from ec4ba19 to 22ca0e8 Compare February 3, 2026 20:58
@ptomato
Copy link
Contributor

ptomato commented Feb 11, 2026

We've tried to divide this up between the folks present in the maintainers meeting, in order to land it:

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

I reviewed adopt/, constructor.js, and toStringTag.js. One comment on constructor.js, but I may just be confused about that.

I'd also suggest adding a test that exercises the following:

const stack = new AsyncDisposableStack();
await stack.disposeAsync();
stack.adopt({}, async () => {});
  // Should throw a ReferenceError

I see this exists already for move() in move/throws-if-disposed.js, but could be added for adopt() and defer(), as well as for DisposableStack. If you want to do that in a separate PR I could review and merge that independently of this PR.

…r, fix internal slot reference in descriptions
@rbuckton
Copy link
Contributor Author

If you want to do that in a separate PR I could review and merge that independently of this PR.

Added here, hope that's not too much noise. If it is I can revert and pull that out to a separate PR.

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

Review for two more prototype methods, here

@jugglinmike
Copy link
Contributor

I've suggested a few tests for the "Await(undefined)" in the DisposeResources algorithm: rbuckton#1

@rbuckton
Copy link
Contributor Author

@gibson042, @ioannad any chance on final reviews this week? It's very possible I will be able to wrap up the ecma262 PR before plenary, which would leave this review as the final blocker to Stage 4.

@ioannad
Copy link
Contributor

ioannad commented Mar 3, 2026

TLDR: defer and move LGTM with a couple of optional additions that are covered in staging, and an extra new line nit.


For step 3 of defer and move I think we also need async tests, to verify that we don't have a race condition, where [[AsyncDisposableState]] hasn't changed yet. There are such tests in staging this one for defer and this one for move) that test exactly that. So I think these could be moved from staging in a different PR, along with the similarly named staging test files for adopt and dispose.

Otherwise the tests seem to completely cover the spec steps for defer and move, thanks!

It would be nice to optionally also have some basic funcionality tests (these are also in staging for defer and for move).

I was also going to suggest a couple of exception propagation tests, but I see these are covered in disposeAsync. :)

…-asyncdisposablestack-that-contains-moved-resources.js

Co-authored-by: Ioanna M Dimitriou H <idimitriou@igalia.com>
@rbuckton
Copy link
Contributor Author

rbuckton commented Mar 6, 2026

Is there a process in place for moving tests out of staging in general? Is there a reason we don't move all of the resource management tests out of staging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants