Swaps template literals for sprintf style interpolation #200634
Merged
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
botelastic
bot
added
ci:project-deploy-observability
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
jasonrhodes
added
release_note:skip
⏳ Build in-progress
Failed CI StepsHistory
|
maryam-saeidi
approved these changes
Nov 19, 2024
Unfortunately I think not, because you can't test the output of stderr. I was able to "spy" on console.error and verify what gets passed to it, but that's not the same as verifying that the log message is formatted the same, which is what I'd really like to test. |
|
Starting backport for target branches: 8.x |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Nov 19, 2024
## Summary String handling in error log output is now managed by console.error internals, for maximum safety re: external input. ## Comparison * The result of `formatErrors` is being passed to `toString()` so that the array stringification matches the previous version. Otherwise, it would include the `[ ]` brackets around the array in the new log output. * The `apiUrl` and `response` are still passed to `console.error` as the final two arguments, so they'll continue to be printed the same way as before. Previous output (using a local jest test): <img width="1495" alt="Screenshot 2024-11-18 at 1 24 11 PM" src="https://github.com/user-attachments/assets/af08a471-ed94-4730-b5c7-fffbb3c9c9f9"> Updated output: <img width="1496" alt="Screenshot 2024-11-18 at 1 46 34 PM" src="https://github.com/user-attachments/assets/7523bc5f-b367-4fb8-8dbb-e16c8f4fd43f"> The local jest test I used to confirm this was this: ```ts import { HttpSetup } from '@kbn/core-http-browser'; import { apiService } from './api_service'; import * as rt from 'io-ts'; describe('API service', () => { it('should log the right error', async () => { const mockHttp = { fetch: jest.fn(async () => ({ data: { myKey: 'myvalue' } })) } as unknown as HttpSetup; apiService.http = mockHttp; const ResponseType = rt.type({ data: rt.string }); await apiService.get( '/my/api/path', {}, ResponseType ); }); }); ``` and it was in `x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts` To run, I used ```sh node scripts/jest ./x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts ``` The test always passes but it allows you to quickly/easily see the output when the condition fails. --------- Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit 0c66148)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Nov 19, 2024
… (#200737) # Backport This will backport the following commits from `main` to `8.x`: - [Swaps template literals for sprintf style interpolation (#200634)](#200634) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Jason Rhodes","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-11-19T14:02:09Z","message":"Swaps template literals for sprintf style interpolation (#200634)\n\n## Summary\r\n\r\nString handling in error log output is now managed by console.error\r\ninternals, for maximum safety re: external input.\r\n\r\n## Comparison\r\n\r\n* The result of `formatErrors` is being passed to `toString()` so that\r\nthe array stringification matches the previous version. Otherwise, it\r\nwould include the `[ ]` brackets around the array in the new log output.\r\n* The `apiUrl` and `response` are still passed to `console.error` as the\r\nfinal two arguments, so they'll continue to be printed the same way as\r\nbefore.\r\n\r\nPrevious output (using a local jest test):\r\n<img width=\"1495\" alt=\"Screenshot 2024-11-18 at 1 24 11 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/af08a471-ed94-4730-b5c7-fffbb3c9c9f9\">\r\n\r\nUpdated output:\r\n<img width=\"1496\" alt=\"Screenshot 2024-11-18 at 1 46 34 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/7523bc5f-b367-4fb8-8dbb-e16c8f4fd43f\">\r\n\r\n\r\nThe local jest test I used to confirm this was this:\r\n\r\n```ts\r\nimport { HttpSetup } from '@kbn/core-http-browser';\r\nimport { apiService } from './api_service';\r\nimport * as rt from 'io-ts';\r\n\r\ndescribe('API service', () => {\r\n\r\n it('should log the right error', async () => {\r\n const mockHttp = { \r\n fetch: jest.fn(async () => ({\r\n data: {\r\n myKey: 'myvalue'\r\n }\r\n })) \r\n } as unknown as HttpSetup;\r\n\r\n apiService.http = mockHttp;\r\n\r\n const ResponseType = rt.type({\r\n data: rt.string\r\n });\r\n\r\n await apiService.get(\r\n '/my/api/path', \r\n {}, \r\n ResponseType\r\n );\r\n });\r\n\r\n});\r\n```\r\nand it was in\r\n`x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts`\r\n\r\nTo run, I used\r\n\r\n```sh\r\nnode scripts/jest ./x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts\r\n```\r\n\r\nThe test always passes but it allows you to quickly/easily see the\r\noutput when the condition fails.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"0c66148fc8480fa3fe844d0f304745dc4b62b946","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-management"],"title":"Swaps template literals for sprintf style interpolation","number":200634,"url":"https://github.com/elastic/kibana/pull/200634","mergeCommit":{"message":"Swaps template literals for sprintf style interpolation (#200634)\n\n## Summary\r\n\r\nString handling in error log output is now managed by console.error\r\ninternals, for maximum safety re: external input.\r\n\r\n## Comparison\r\n\r\n* The result of `formatErrors` is being passed to `toString()` so that\r\nthe array stringification matches the previous version. Otherwise, it\r\nwould include the `[ ]` brackets around the array in the new log output.\r\n* The `apiUrl` and `response` are still passed to `console.error` as the\r\nfinal two arguments, so they'll continue to be printed the same way as\r\nbefore.\r\n\r\nPrevious output (using a local jest test):\r\n<img width=\"1495\" alt=\"Screenshot 2024-11-18 at 1 24 11 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/af08a471-ed94-4730-b5c7-fffbb3c9c9f9\">\r\n\r\nUpdated output:\r\n<img width=\"1496\" alt=\"Screenshot 2024-11-18 at 1 46 34 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/7523bc5f-b367-4fb8-8dbb-e16c8f4fd43f\">\r\n\r\n\r\nThe local jest test I used to confirm this was this:\r\n\r\n```ts\r\nimport { HttpSetup } from '@kbn/core-http-browser';\r\nimport { apiService } from './api_service';\r\nimport * as rt from 'io-ts';\r\n\r\ndescribe('API service', () => {\r\n\r\n it('should log the right error', async () => {\r\n const mockHttp = { \r\n fetch: jest.fn(async () => ({\r\n data: {\r\n myKey: 'myvalue'\r\n }\r\n })) \r\n } as unknown as HttpSetup;\r\n\r\n apiService.http = mockHttp;\r\n\r\n const ResponseType = rt.type({\r\n data: rt.string\r\n });\r\n\r\n await apiService.get(\r\n '/my/api/path', \r\n {}, \r\n ResponseType\r\n );\r\n });\r\n\r\n});\r\n```\r\nand it was in\r\n`x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts`\r\n\r\nTo run, I used\r\n\r\n```sh\r\nnode scripts/jest ./x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts\r\n```\r\n\r\nThe test always passes but it allows you to quickly/easily see the\r\noutput when the condition fails.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"0c66148fc8480fa3fe844d0f304745dc4b62b946"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/200634","number":200634,"mergeCommit":{"message":"Swaps template literals for sprintf style interpolation (#200634)\n\n## Summary\r\n\r\nString handling in error log output is now managed by console.error\r\ninternals, for maximum safety re: external input.\r\n\r\n## Comparison\r\n\r\n* The result of `formatErrors` is being passed to `toString()` so that\r\nthe array stringification matches the previous version. Otherwise, it\r\nwould include the `[ ]` brackets around the array in the new log output.\r\n* The `apiUrl` and `response` are still passed to `console.error` as the\r\nfinal two arguments, so they'll continue to be printed the same way as\r\nbefore.\r\n\r\nPrevious output (using a local jest test):\r\n<img width=\"1495\" alt=\"Screenshot 2024-11-18 at 1 24 11 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/af08a471-ed94-4730-b5c7-fffbb3c9c9f9\">\r\n\r\nUpdated output:\r\n<img width=\"1496\" alt=\"Screenshot 2024-11-18 at 1 46 34 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/7523bc5f-b367-4fb8-8dbb-e16c8f4fd43f\">\r\n\r\n\r\nThe local jest test I used to confirm this was this:\r\n\r\n```ts\r\nimport { HttpSetup } from '@kbn/core-http-browser';\r\nimport { apiService } from './api_service';\r\nimport * as rt from 'io-ts';\r\n\r\ndescribe('API service', () => {\r\n\r\n it('should log the right error', async () => {\r\n const mockHttp = { \r\n fetch: jest.fn(async () => ({\r\n data: {\r\n myKey: 'myvalue'\r\n }\r\n })) \r\n } as unknown as HttpSetup;\r\n\r\n apiService.http = mockHttp;\r\n\r\n const ResponseType = rt.type({\r\n data: rt.string\r\n });\r\n\r\n await apiService.get(\r\n '/my/api/path', \r\n {}, \r\n ResponseType\r\n );\r\n });\r\n\r\n});\r\n```\r\nand it was in\r\n`x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts`\r\n\r\nTo run, I used\r\n\r\n```sh\r\nnode scripts/jest ./x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts\r\n```\r\n\r\nThe test always passes but it allows you to quickly/easily see the\r\noutput when the condition fails.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"0c66148fc8480fa3fe844d0f304745dc4b62b946"}}]}] BACKPORT--> Co-authored-by: Jason Rhodes <[email protected]>
paulinashakirova
pushed a commit
to paulinashakirova/kibana
that referenced
this pull request
Nov 26, 2024
## Summary String handling in error log output is now managed by console.error internals, for maximum safety re: external input. ## Comparison * The result of `formatErrors` is being passed to `toString()` so that the array stringification matches the previous version. Otherwise, it would include the `[ ]` brackets around the array in the new log output. * The `apiUrl` and `response` are still passed to `console.error` as the final two arguments, so they'll continue to be printed the same way as before. Previous output (using a local jest test): <img width="1495" alt="Screenshot 2024-11-18 at 1 24 11 PM" src="https://github.com/user-attachments/assets/af08a471-ed94-4730-b5c7-fffbb3c9c9f9"> Updated output: <img width="1496" alt="Screenshot 2024-11-18 at 1 46 34 PM" src="https://github.com/user-attachments/assets/7523bc5f-b367-4fb8-8dbb-e16c8f4fd43f"> The local jest test I used to confirm this was this: ```ts import { HttpSetup } from '@kbn/core-http-browser'; import { apiService } from './api_service'; import * as rt from 'io-ts'; describe('API service', () => { it('should log the right error', async () => { const mockHttp = { fetch: jest.fn(async () => ({ data: { myKey: 'myvalue' } })) } as unknown as HttpSetup; apiService.http = mockHttp; const ResponseType = rt.type({ data: rt.string }); await apiService.get( '/my/api/path', {}, ResponseType ); }); }); ``` and it was in `x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts` To run, I used ```sh node scripts/jest ./x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts ``` The test always passes but it allows you to quickly/easily see the output when the condition fails. --------- Co-authored-by: kibanamachine <[email protected]>
CAWilson94
pushed a commit
to CAWilson94/kibana
that referenced
this pull request
Dec 12, 2024
## Summary String handling in error log output is now managed by console.error internals, for maximum safety re: external input. ## Comparison * The result of `formatErrors` is being passed to `toString()` so that the array stringification matches the previous version. Otherwise, it would include the `[ ]` brackets around the array in the new log output. * The `apiUrl` and `response` are still passed to `console.error` as the final two arguments, so they'll continue to be printed the same way as before. Previous output (using a local jest test): <img width="1495" alt="Screenshot 2024-11-18 at 1 24 11 PM" src="https://github.com/user-attachments/assets/af08a471-ed94-4730-b5c7-fffbb3c9c9f9"> Updated output: <img width="1496" alt="Screenshot 2024-11-18 at 1 46 34 PM" src="https://github.com/user-attachments/assets/7523bc5f-b367-4fb8-8dbb-e16c8f4fd43f"> The local jest test I used to confirm this was this: ```ts import { HttpSetup } from '@kbn/core-http-browser'; import { apiService } from './api_service'; import * as rt from 'io-ts'; describe('API service', () => { it('should log the right error', async () => { const mockHttp = { fetch: jest.fn(async () => ({ data: { myKey: 'myvalue' } })) } as unknown as HttpSetup; apiService.http = mockHttp; const ResponseType = rt.type({ data: rt.string }); await apiService.get( '/my/api/path', {}, ResponseType ); }); }); ``` and it was in `x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts` To run, I used ```sh node scripts/jest ./x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.ts ``` The test always passes but it allows you to quickly/easily see the output when the condition fails. --------- Co-authored-by: kibanamachine <[email protected]>
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.
Summary
String handling in error log output is now managed by console.error internals, for maximum safety re: external input.
Comparison
formatErrorsis being passed totoString()so that the array stringification matches the previous version. Otherwise, it would include the[ ]brackets around the array in the new log output.apiUrlandresponseare still passed toconsole.erroras the final two arguments, so they'll continue to be printed the same way as before.Previous output (using a local jest test):
Updated output:
The local jest test I used to confirm this was this:
and it was in
x-pack/plugins/observability_solution/synthetics/public/utils/api_service/api_service.test.tsTo run, I used
The test always passes but it allows you to quickly/easily see the output when the condition fails.