Skip to content

fix(core): add missing return for instantiated exposed classes#529

Merged
omermorad merged 4 commits into
nextfrom
fix/sociable-with-empty-constructor-params
Jan 2, 2025
Merged

fix(core): add missing return for instantiated exposed classes#529
omermorad merged 4 commits into
nextfrom
fix/sociable-with-empty-constructor-params

Conversation

@craig0990

@craig0990 craig0990 commented Dec 5, 2024

Copy link
Copy Markdown
Collaborator

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

Return early when instantiating a class to be exposed; which prevents falling through to the mockFunction call.

  • Bugfix

What is the current behavior?

Issue Number: #509

What is the new behavior?

Does this PR introduce a breaking change?

  • No

Other information

@craig0990 craig0990 requested a review from omermorad December 5, 2024 13:37
@codecov

codecov Bot commented Dec 5, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.52%. Comparing base (f16c747) to head (30a8d35).
Report is 18 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next     #529      +/-   ##
==========================================
+ Coverage   89.76%   92.52%   +2.76%     
==========================================
  Files           6       26      +20     
  Lines         127      589     +462     
  Branches       25       99      +74     
==========================================
+ Hits          114      545     +431     
- Misses         10       35      +25     
- Partials        3        9       +6     
Flag Coverage Δ
core.unit 93.44% <100.00%> (?)
di.inversify 93.68% <ø> (?)
di.nestjs 87.25% <ø> (ø)
doubles.jest 100.00% <ø> (?)
doubles.sinon 100.00% <ø> (ø)
doubles.vitest 100.00% <ø> (?)
unit 77.27% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@omermorad omermorad changed the base branch from master to next December 17, 2024 21:13
@omermorad omermorad changed the title test: Add test case for constructors issue #509 test: add test case for constructors issue #509 Dec 17, 2024
@omermorad

Copy link
Copy Markdown
Collaborator

@craig0990 I have fixed the issue :)
Ran the e2e workflow with your changes and now we can see the failing tests: https://github.com/suites-dev/suites/actions/runs/12381566430/job/34560407726
Let me know if it helps

@craig0990

Copy link
Copy Markdown
Collaborator Author

Thanks @omermorad - seeing it failing in E2E did get me past the "what am I missing" stage; although I'll admit that locally I hijacked the e2e suite by adding it to the root workspaces to get linked dependencies to play with.

It looks like a missing return value to me, which I've added in the latest commit. I can see that the codecov needs looking at, so will make that the next thing to take a look at, and potentially the same E2E test but for some of the other DI suites.

@craig0990 craig0990 changed the title test: add test case for constructors issue #509 fix(core): Add missing return for instantiated exposed classes #509 Dec 23, 2024
@omermorad omermorad changed the title fix(core): Add missing return for instantiated exposed classes #509 fix(core): add missing return for instantiated exposed classes #509 Dec 23, 2024
@omermorad omermorad changed the title fix(core): add missing return for instantiated exposed classes #509 fix(core): add missing return for instantiated exposed classes Dec 23, 2024
@omermorad

omermorad commented Dec 23, 2024

Copy link
Copy Markdown
Collaborator

Thanks @omermorad - seeing it failing in E2E did get me past the "what am I missing" stage; although I'll admit that locally I hijacked the e2e suite by adding it to the root workspaces to get linked dependencies to play with.

It looks like a missing return value to me, which I've added in the latest commit. I can see that the codecov needs looking at, so will make that the next thing to take a look at, and potentially the same E2E test but for some of the other DI suites.

@craig0990
E2E is our hero! 🦸🏻
I've noticed the absence of the return value, and the e2e test is now successful 🎉
Regarding the code coverage, I recommend checking out this link. It contains the integration test we should utilize to address the gaps.

I have some reservations regarding the other e2e tests, as we're focusing on core functionality rather than aspects related to dependency injection or mocking libraries, maybe we should just cover in nestjs-jest

@craig0990 craig0990 force-pushed the fix/sociable-with-empty-constructor-params branch from 0961941 to 59e6434 Compare January 1, 2025 08:50
@craig0990 craig0990 marked this pull request as ready for review January 1, 2025 10:52
@omermorad omermorad force-pushed the next branch 4 times, most recently from 562a960 to 5f7b0da Compare January 1, 2025 16:27
@omermorad

Copy link
Copy Markdown
Collaborator

@craig0990 There appears to be a problem with the commits due to an error I made with the next branch. Could you please open a new PR?

@craig0990

Copy link
Copy Markdown
Collaborator Author

@omermorad I can rebase it, next is the correct target branch right?

@craig0990 craig0990 force-pushed the fix/sociable-with-empty-constructor-params branch from 59e6434 to e0d8798 Compare January 2, 2025 11:13
@omermorad

Copy link
Copy Markdown
Collaborator

@craig0990 Awesome! LGTM! :)
Fixed some lint..

@omermorad omermorad merged commit de7f80d into next Jan 2, 2025
@omermorad omermorad deleted the fix/sociable-with-empty-constructor-params branch January 2, 2025 17:10
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.

2 participants