Skip to content

test(signals): improve signalMethod injector tests #4798

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

Conversation

iliev-ilian
Copy link
Contributor

Small improvements in signal method tests:

  • Test "does not cause issues if destroyed signalMethodFn contains destroyed effectRefs" - summand2 should be set with different value so the effect is triggered for summand2 and it is not triggered for summand1, which uses the detroyed Injector. Otherwise it causes confusion that may be also the second summand is not "observed" from the effect. It is, but the set is called with the same value.
  • Test "uses the provided injector (source injector) on creation" - one small additional same expect to show that after the destroy of Injector, the signal is not tracked in the effect.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ x] No

Other information

Copy link

netlify bot commented May 27, 2025

Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit 49aaf10
🔍 Latest deploy log https://app.netlify.com/projects/ngrx-io/deploys/68357031cec4dd0008bf798d

Copy link

netlify bot commented May 27, 2025

Deploy Preview for ngrx-site-v19 ready!

Name Link
🔨 Latest commit 49aaf10
🔍 Latest deploy log https://app.netlify.com/projects/ngrx-site-v19/deploys/68357031511a340008e2e0db
😎 Deploy Preview https://deploy-preview-4798--ngrx-site-v19.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@iliev-ilian
Copy link
Contributor Author

@rainerhahnekamp can you please have a look?

Copy link
Contributor

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

Yes, that's an improvement to the unit tests.

@markostanimirovic markostanimirovic changed the title refactor(signals): improve signalMethod injector tests test(signals): improve signalMethod injector tests May 28, 2025
@markostanimirovic markostanimirovic merged commit 70a65fe into ngrx:main May 28, 2025
14 checks passed
@markostanimirovic
Copy link
Member

Thanks @iliev-ilian!

rainerhahnekamp pushed a commit to rainerhahnekamp/ngrx that referenced this pull request Jun 1, 2025
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.

3 participants