refactor: remove test failures/logs from injection token maker #904
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @davidlj95 and the rest of your teammates on |
davidlj95
force-pushed
the
stacked/refactor-remove-log-test-failures-from-injection-token-maker
branch
from
a4d72c1 to
7b3c91b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #904 +/- ##
=======================================
Coverage 99.46% 99.46%
=======================================
Files 79 79
Lines 372 372
Branches 73 73
=======================================
Hits 370 370
Partials 2 2 ☔ View full report in Codecov by Sentry. |
📦 Bundle size (Angular v18)Git ref:
|
📦 Bundle size (Angular v15)Git ref:
|
📦 Bundle size (Angular v16)Git ref:
|
📦 Bundle size (Angular v17)Git ref:
|
davidlj95
force-pushed
the
stacked/refactor-remove-log-test-failures-from-injection-token-maker
branch
from
7b3c91b to
b6e54e4
Compare
davidlj95
changed the title
davidlj95
changed the title
davidlj95
force-pushed
the
stacked/refactor-remove-log-test-failures-from-injection-token-maker
branch
from
b6e54e4 to
d914993
Compare
davidlj95
commented
Oct 9, 2024
| INJECTION_TOKENS.clear() | ||
| INJECTION_TOKEN_FACTORIES.clear() | ||
| INJECTION_TOKENS.delete(description) | ||
| INJECTION_TOKEN_FACTORIES.delete(description) |
There was a problem hiding this comment.
Not actually needed. However nice to only mess one dummy injection token just in case.
davidlj95
force-pushed
the
stacked/refactor-remove-log-test-failures-from-injection-token-maker
branch
from
d914993 to
95f8ff9
Compare
davidlj95
commented
Oct 9, 2024
| @@ -27,9 +27,13 @@ export const _makeInjectionToken: <T>( | |||
| const injectionToken = | |||
| INJECTION_TOKENS.get(description) ?? | |||
| new InjectionToken(`ngx-meta ${description}`, { factory }) | |||
| INJECTION_TOKENS.set(description, injectionToken) | |||
There was a problem hiding this comment.
Moved here so the prod code is ran as soon as possible (after thinking about race conditions and so). Though not needed actually.
davidlj95
force-pushed
the
stacked/refactor-remove-log-test-failures-from-injection-token-maker
branch
from
95f8ff9 to
3049da7
Compare
davidlj95
deleted the
stacked/refactor-remove-log-test-failures-from-injection-token-maker
branch
|
🎉 This PR is included in version 1.0.0-beta.17 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue or need
Moar issues appeared after trying to use lazy injection tokens in #902:
Proposed changes
Removes warning. As after usingThinking about it again, doesn't make sense. Given if inspected, source code string is the same. Tests ran on the browser. All of them. Test setup code included. So code should be the same. The issue was that in #902 , the injection token factory is an anonymous function. That gets created when calling the function to create the lazy injection token. So its identity is different every time. Therefore we can't check for object identity and that's it. Therefore in order to check if they're the same or not, usingtoString()on factories for injection tokens, seems the issue may be due to tests running on Node.js but executing on the browser. Where code may be slightly different after bundling.toString()to compare source codes. Added a unit test to ensure about that.Deletes only the
dummyinjection token created for unit testing. Otherwise reseting the whole injection tokens map messes up tests. Why though? 🤔After a few times trying this on top of #902 , seems that first time clearing, the head element upsert or remove util injection token is there. And if clearing them, some times unit tests fail. Not always though. What may be happening is a race condition. The injection token gets created and test starts. Then,
clearhappens and removes it. Then, the test code tries to inject that token. But as it's been cleared, it creates a new one. The new one isn't a spy. So the spy isn't called. Deleting only the dummy injection token solves this.This PR has been tested on top of #902 to ensure everything works fine when the API is used around.
Quick reminders