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.

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

feat(cli): add test permissions to Deno.test #10188

Merged
merged 36 commits into from
Apr 25, 2021

Conversation

caspervonb
Copy link
Contributor

@caspervonb caspervonb commented Apr 14, 2021

This adds adds permissions to the test definitions which allows tests to run with different permission sets than the process's permission.

The change will only be in effect within the test function, once the test has completed the original process permission set is restored.

Test permissions cannot exceed the process's permission.
You can only narrow or drop permissions, failure to acquire a permission results in an error being thrown and the test case will fail.

Closes #9859

@caspervonb caspervonb changed the title feat(cli): add test permissions to Deno.TestDefinition feat(cli): add test permissions to Deno.test Apr 14, 2021
cli/tests/test/allow_all.ts Outdated Show resolved Hide resolved
cli/dts/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/main.rs Outdated Show resolved Hide resolved
cli/ops/test_runner.rs Outdated Show resolved Hide resolved
@bartlomieju bartlomieju added this to the 1.10.0 milestone Apr 16, 2021
@piscisaureus
Copy link
Member

@caspervonb Whatever happens - thanks for you contribution, we appreciate the time & effort you're putting into Deno!

That said, I don't really understand why this change is necessary. Maybe I'm just ignorant. However, can you give me the layman's explanation?

@caspervonb
Copy link
Contributor Author

caspervonb commented Apr 24, 2021

Thanks @piscisaureus.

This basic premise is to set up the permissions for a specific test case.

We can't do this with Deno.permissions.revoke because once permission is revoked you can't go back so there's no way to set up a test case with a certain set of permissions with the current permission API (@bartlomieju was kicking around the idea of adding it to Deno.permissions but this seemed more appropriate to me as it limits the impact to a very specific area).

The question of "How do I run a test case with x, y, and z" permissions does come up every now and then altho not that frequently it is something users find themselves needing when working with APIs that require certain permissions.

We set up these preconditions internally in our unit tests by not using Deno.test or even the test command at all so this simplifies the whole ordeal quite a bit and allows us to drop the whole unit test runner and just dogfood Deno.test.

The existence of the unit test runner is causing us to leak a bunch of internal details about the test runner as well which is not great.


TL;DR it is needed to set up the preconditions for a test that expects to have X permissions.

As a side effect, this will let us drop a few thousand lines of code that is used to support and implement our custom ad-hoc unit test runner and dogfood the user-facing test runner in our own test suite.

cli/ops/test_runner.rs Outdated Show resolved Hide resolved
cli/ops/test_runner.rs Outdated Show resolved Hide resolved
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Cleanly implemented. I think it's ready to land after addressing Bert's question regarding JSdoc. Nice work @caspervonb

cli/ops/test_runner.rs Outdated Show resolved Hide resolved
cli/ops/test_runner.rs Show resolved Hide resolved
Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bartlomieju bartlomieju 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 f3751e4 into denoland:main Apr 25, 2021
@caspervonb caspervonb deleted the feat-add-test-permissions branch April 28, 2021 12:35
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.

Add Permissions to TestDefinition
4 participants