fix: apply client logging options directly on client loading #2946
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2946 +/- ##
==========================================
+ Coverage 92.39% 92.84% +0.44%
==========================================
Files 37 38 +1
Lines 1263 1271 +8
Branches 328 330 +2
==========================================
+ Hits 1167 1180 +13
+ Misses 91 87 -4
+ Partials 5 4 -1
Continue to review full report at Codecov.
|
| function getSocketUrl(urlParts, loc) { | ||
| function createSocketUrl(urlParts, loc) { | ||
| // Use parameter to allow passing location in unit tests | ||
| if (typeof loc === 'string' && loc !== '') { |
There was a problem hiding this comment.
Yep, it is. I kept it how it was before. (I've tried removing it and overriding self.location in the test but that didn't work. Probably because self doesn't work as expected in the test environment?)
There was a problem hiding this comment.
hm, weird, self should work, jest allows to mock this
There was a problem hiding this comment.
I guess that's why it has already been mocked before
There was a problem hiding this comment.
I think we should avoid code for testing from runtime...
For Bugs and Features; did you add new tests?
Updating logs snapshots was enough to test the new expected behavior. Some client tests were refactored as a result of some refactor in the client utils.
Motivation / Use-Case
Following the issue reported in #2932, client logging options needed to be moved to initial client options (the ones passed in the entry URL query).
Breaking Changes
None
Additional Info
Since the query parsing was done directly in the socket URL generation function, it had to be moved to a separate util. This split also simplified the unit tests of those utils.