Uh oh!
There was an error while loading. Please reload this page.
Firefox and Safari integration tests fixes - #1029
Conversation
987851b to
f8ac9e2Compare| // E.g. throwing a string or raw object in Firefox | ||
| // Generating synthetic error doesn't add any value here | ||
| // | ||
| // We should probably somehow let user know that he should fix his code |
There was a problem hiding this comment.
should say "they should fix their code"
There was a problem hiding this comment.
in_app and filename are the only changed keys here, right? would it be simpler to just make the normalized object assign filename : frame.url || stackInfoUrl, and rely on the existing logic for in_app? or does in_app actually return a false positive in this case?
There was a problem hiding this comment.
Correct. Just wanted to be a little more explicit, as we don't have function nor filename which are used in tests for in_app logic. I still left it on a separate line now though, as it's more readable with this long comment this way.
| view: window | ||
| } | ||
| if ('MouseEvent' in window) { |
There was a problem hiding this comment.
wouldn't hurt to comment these branches with the browsers they're active on
There was a problem hiding this comment.
these factories are way better though 👍
MaxBittker
left a comment
There was a problem hiding this comment.
definitely fix the comment, the normalizedFrame thing being cleaner or not is up to you. Exciting to have green tests cross browser, this is long overdue! 💯
| // E.g. throwing a string or raw object in Firefox | ||
| // Generating synthetic error doesn't add any value here | ||
| // | ||
| // We should probably somehow let user know that he should fix his code |
There was a problem hiding this comment.
in_app and filename are the only changed keys here, right? would it be simpler to just make the normalized object assign filename : frame.url || stackInfoUrl, and rely on the existing logic for in_app? or does in_app actually return a false positive in this case?
| "test": "npm run lint && grunt build.test && npm run test:unit && npm run test:integration && npm run test:typescript", | ||
| "test:unit": "mocha-chrome test/index.html", | ||
| "test:integration": "mocha-chrome test/integration/index.html --chrome-flags '[\"--disable-web-security\"]' --ignore-resource-errors --ignore-exceptions", | ||
| "test:typescript": "tsc --noEmit --noImplicitAny typescript/raven-tests.ts" |
There was a problem hiding this comment.
I generally link linking to the version found in node_modules because collaborators often don't have node_modules/.bin on their PATH.
There was a problem hiding this comment.
I think when you run things using yarn/npm, it'll add node_modules/.bin to search path.
benvinegar
commented
Sep 7, 2017
Dismissing @MaxBittker's review because his comments appear addressed and this seems good to merge. |
- Fix MouseEvents related integration tests on Chrome - Fix Mouse/KeyboardEvents related integration tests on Firefox - Update captureException test assertion for Safari - Fix non-error throws in onerror handler on Firefox - Simplify _normalizeFrame edgecase and comment on event factories
- Use Headless Chrome instead of PhantomJS - Reconfigure TravisCI to utilize new setup - Remove PhantomJS guards in integration tests - Start partial migration to npm scripts instead of Grunt - Remove lodash and use native functions instead - Remove redundant packages
ece5380 to
bafa99cComparekamilogorek
commented
Sep 8, 2017
Rebased and merged |
A PR closing this issue has just been released 🚀This issue was referenced by PR #14643, which was included in the 8.45.0 release. |
There are 2 considerations we have to take into account.
In
Update captureException test assertion for Safaricommit, I decreased required frames to1, as Safari is not able to gather any more information about manually caught errors coming fromnon-errorsource, eg. strings/object. We need "at least"captureMessagecall with appropriate message attached, as this is whatcaptureExceptiondefaults to when passing non-error argument to it.In
Fix non-error throws on onerror handler on FirefoxI modifiednormalizeFramesmethod to account for the worst case possible. Quoting my comment in the code:I'd appreciate feedback on both of those issue.
When this PR and #1026 get merged, we'll be all green on Chrome, Firefox and Safari on OSX and we'll be able to move forward with more tests.
NOTE: I have to update Input related tests on Phantom.js (will do that first time in the morning)