Improve Logger class #397
Improve Logger class #397
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
| expect(mockServerLoggingCallback).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| api_setCookie: false, | ||
| logPacket: expect.any(String), |
There was a problem hiding this comment.
These tests are now a bit shitty, since the logPacket contains a timestamp, I don't know how to make this test a bit better (like check its including the actual log lines).
There was a problem hiding this comment.
jest functions can have their arguments inspected so that might be one way?
e.g. mockServerLoggingCallback.mock.calls should return an array of arrays representing each calls arguments. So to get the first argument of the first call you'd do
mockServerLoggingCallback.mock.calls[0][0]There was a problem hiding this comment.
And then we can do like expect(args).toEqual({something: 'blah'})
There was a problem hiding this comment.
Oh nice, did not know that, will try it
| @@ -81,60 +73,51 @@ export default class Logger { | |||
| * @param {String} message The message to log. | |||
| * @param {Boolean} sendNow if true, the message will be sent right away. | |||
| * @param {Object|String} parameters The parameters to send along with the message | |||
| * @param {Boolean} onlyFlushWithOthers A request will never be sent to the server if all loglines have this set to true | |||
There was a problem hiding this comment.
I added this param for 2 reasons:
- I did not want it to be sending a request to the server with only this message as that would be useless
- Same thing happens with this log line
|
Please review this even if on hold. Once this is approved, I need to update all the PRs in all the repos using this. |
iwiznia
requested review from
marcaaron and
a team
and removed request for
Luke9389
|
Bump! |
| expect(mockServerLoggingCallback).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| api_setCookie: false, | ||
| logPacket: expect.any(String), |
There was a problem hiding this comment.
jest functions can have their arguments inspected so that might be one way?
e.g. mockServerLoggingCallback.mock.calls should return an array of arrays representing each calls arguments. So to get the first argument of the first call you'd do
mockServerLoggingCallback.mock.calls[0][0]| expect(mockServerLoggingCallback).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| api_setCookie: false, | ||
| logPacket: expect.any(String), |
There was a problem hiding this comment.
And then we can do like expect(args).toEqual({something: 'blah'})
|
Updated |
|
Had to update this to fix a bug when testing Expensify/App#4191 |
|
ok, I updated this and am working through to update and re-test the other PRs, please approve. I will merge them all once they are all ready. |
| return this.logLines.slice(this.logLines.length - count); | ||
| // If we're over the limit, flush the logs | ||
| if (length > MAX_LOG_LINES_BEFORE_FLUSH || forceFlushToServer) { | ||
| this.logToServer() |
Alright, please give this a review. You can see it being used in the draft PRs tied to the issue.
I basically revamped how the logging class works, mainly:
This is an example of what we log in App (there's 2 requests there to see the previous requestID thingy):
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/171640
Tests
QA