-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Append Elastic/synthetics to UA string #380
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though this works, This would be the last approach we would need to do as it slows down the startup time of our runner. Please look in to - https://chromedevtools.github.io/devtools-protocol/tot/Browser/#method-getVersion
which can be used to get the version and can be used for setting the global user agent context. Let me know if you need any help.
We discussed offline and decided to go with using browser based session approach to get the user agent. Benchmarks if anyone is curious
|
i tested manualy, it seems to work with iframe and new window as well. will try to write some tests for those use cases. |
Added tests for iframe and popup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good, good to go after removing some bad import statements.
__tests__/core/gatherer.test.ts
Outdated
await Gatherer.stop(); | ||
}); | ||
|
||
it('works with iframe content', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove this iframe test in favor of request test. Or merge it on the previous popup test to look for request to popup.html
and check if request.headers.userAgent contains the Elastic identifier.
Iframes tests are unnecessary as they inherit the parent document.
|
||
jest.mock('../../src/plugins/network'); | ||
|
||
describe('Gatherer', () => { | ||
let server: Server; | ||
|
||
beforeAll(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Close the server on afterAll.
__tests__/core/gatherer.test.ts
Outdated
@@ -26,10 +26,19 @@ | |||
import { Gatherer } from '../../src/core/gatherer'; | |||
import { PluginManager } from '../../src/plugins'; | |||
import { wsEndpoint } from '../utils/test-config'; | |||
import { devices } from 'playwright-chromium'; | |||
import { Server } from '../utils/server'; | |||
import exp = require('constants'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bad import.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Fixes #232
Appends Elastic/Synthetics to UA string.
This PR uses a pretty ugly way to determin current user agent string and append the desired string into it.
Not sure if there is a way better way to do this. Struggling with playwright docs on how to get the current user agent string without opening a browser context.