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

test: return frozen function from common.must[Not]Call #43825

Closed

Conversation

LiviaMedeiros
Copy link
Contributor

I wonder if it would make sense to return frozen functions from mustCall(), mustCallAtLeast(), mustNotCall() and mustSucceed().

Pros: implicit check that functions are never mutated, with no code cost.

Cons: impossible to monkey-patch them in tests.

Affects `mustCall()`, `mustCallAtLeast()`, `mustNotCall()`, and
`mustSucceed()`.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 13, 2022
@tniessen
Copy link
Member

On first glance, this seems more surprising than useful to me.

@F3n67u F3n67u added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jul 25, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 25, 2022
@nodejs-github-bot
Copy link
Collaborator

@F3n67u F3n67u added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 25, 2022
@LiviaMedeiros
Copy link
Contributor Author

Some additional elaboration on the purpose here: this PR is partially a continuation of #43196 and attempt to work around its inability work with functions.
Functions are mutable, and we almost never want them to be mutated. An accidental mutation is possible, for example, if we pass a callback to "overloaded" method with complicated logic to determine which "signature" is needed. It can potentially happen only in case of error, without calling the callback - making it harder to catch.
Fortunately, most of the time tests are using common.mustNotCall() or functions wrapped into common.mustCall() or common.mustSucceed(). Freezing returned functions allows us to:

  • Ensure that test code doesn't mutate functions by wrapping them
  • Have this implicit check across a lot of places in already existing tests

This is very implicit so I'm not 100% sure about this idea; feel free to block if it sounds dangerous or completely useless.

@F3n67u F3n67u added review wanted PRs that need reviews. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 25, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jul 26, 2022

On first glance, this seems more surprising than useful to me.

I agree, the name doesn't give any indication that this might happen, and it feels weird that there is no way of disabling it.

@jasnell
Copy link
Member

jasnell commented Aug 28, 2022

Not really seeing the usefulness here.

@LiviaMedeiros
Copy link
Contributor Author

Closing as unneeded. Adjustments to test-timers-user-call.js might make the test more strict but it won't improve what is tested. Freezing functions might be done independently to perform overall more strict runs, but it doesn't fit main branch. Thanks for reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants