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

feat: Stabilize Deno.TestDefinition.permissions #12078

Merged
merged 7 commits into from Oct 31, 2021
Merged

feat: Stabilize Deno.TestDefinition.permissions #12078

merged 7 commits into from Oct 31, 2021

Conversation

bartlomieju
Copy link
Member

Closes #12025

@bartlomieju bartlomieju added this to the 1.15.0 milestone Sep 14, 2021
dsherret
dsherret requested changes Sep 30, 2021
View reviewed changes
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

I think we need to update op_pledge_test_permissions to not do an unstable check?

dsherret
dsherret reviewed Sep 30, 2021
View reviewed changes
cli/dts/lib.deno.ns.d.ts
*
* Defaults to "inherit".
*/
run?: "inherit" | boolean | Array<string | URL>;
Copy link
Member

Choose a reason for hiding this comment

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

I was looking at parsePermissions in 11_workers.js and run seems like it does not support an array even though it says it does here. The documentation comment also doesn't describe what an array does.

I'm a little concerned about this API not being well tested considering there are these major issues with it (like the other one I discovered and fixed). Perhaps we should work on improving the test coverage for this feature and worker permissions (if anything, at least we could open an issue).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like I accidentally fixed this in #11981. I'll extract the fix and cleanups to a separate PR.

@nayeemrmn nayeemrmn mentioned this pull request Oct 2, 2021
Merged
@bartlomieju
Copy link
Member Author

Blocked by #12297

@bartlomieju bartlomieju added the testing related to deno test and coverage label Oct 8, 2021
@lucacasonato lucacasonato removed this from the 1.15.0 milestone Oct 11, 2021
@CLAassistant
Copy link

CLAassistant commented Oct 15, 2021 •
edited

CLA assistant check
All committers have signed the CLA.

@bartlomieju bartlomieju added this to the 1.16.0 milestone Oct 30, 2021
dsherret
dsherret approved these changes Oct 31, 2021
View reviewed changes
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju bartlomieju merged commit 61e9bea into denoland:main Oct 31, 2021
@bartlomieju bartlomieju deleted the stabilize_test_permissions branch October 31, 2021 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nayeemrmn nayeemrmn nayeemrmn left review comments

@dsherret dsherret dsherret approved these changes

@lucacasonato lucacasonato Awaiting requested review from lucacasonato

Assignees
No one assigned
Labels
testing related to deno test and coverage
Projects
None yet
Milestone
1.16.0
Development

Successfully merging this pull request may close these issues.

Stabilise Deno.TestDefinition.permissions
5 participants
@bartlomieju @CLAassistant @dsherret @nayeemrmn @lucacasonato