Implement fixes in Raygun4JS to correctly handle XHRs and virtual pages + enables tests #491
Conversation
darcythomas
changed the title
darcythomas
changed the title
There was a problem hiding this comment.
Have completed my review of the changes. I'm a little concerned about the test timing blowing out or becoming flakey due to the async behaviour implemented, how come we added all this? Is it just to get GH actions to work?
Otherwise just some minor code formatting changes, I wonder if there is a eslint file in this project to standardise how IDEs apply code styles
| "grunt-contrib-concat": "^1.0.1", | ||
| "grunt-contrib-jshint": "^2.1.0", | ||
| "grunt-contrib-uglify": "^4.0.1", | ||
| "@wdio/cli": "^8.11.2", |
There was a problem hiding this comment.
What led to all these dependencies needing to be updated?
There was a problem hiding this comment.
Well!
I needed to update the node version to one which was supported:
- Period
- On My machine
- On Github actions
I then had to update the testing libraries etc. So that would compile under a supported version of node.
Because of a breaking changes in the wdio & jasmine i needed to update thew tests (async and removing .not. respectively)
| } else if (window.XDomainRequest) { | ||
| xhr.ontimeout = function() { | ||
| if (_enableOfflineSave) { | ||
| Raygun.Utilities.log('Raygun: saved locally'); | ||
| offlineSave(url, data); | ||
| } | ||
| }; |
There was a problem hiding this comment.
please explain this regression?
There was a problem hiding this comment.
Fair.
This was a missed piece for another PR.
window.XDomainRequest has been obsoleted since IE8.
So I removed it from here as well.
| browser.reloadSession(); | ||
| beforeEach(function() { | ||
| originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; | ||
| jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000; |
There was a problem hiding this comment.
what is the test execution time impact of this change?
There was a problem hiding this comment.
Not much this only applies for these few tests. and most of them complete will within the timeout period.
It takes about 5 min to complete all of the tests
There was a problem hiding this comment.
To took about 5 min the last time these tests were run successfully
There was a problem hiding this comment.
Ok i have made this only pause as long as it needs to (up to 40 seconds)
There was a problem hiding this comment.
sounds good, seeing as these are only being run by GH actions this won't have an impact on any TC builds. Happy for the added test time due to the async behaviour 😄
| expect(payloadUrl === pairUrl).toBeTrue("failed for type: " + pairType); | ||
| expect(payloadStatus === pairStatus).toBeTrue( "failed for type: " + pairType); | ||
| expect(payloadDataType === payloadDataType).toBeTrue( "XHR data type missing for: " + pairType); |
There was a problem hiding this comment.
we have lost the jasmine comparitors here?
There was a problem hiding this comment.
Yes. It throws a 'Negation comparison not an option' (or some thing like that).
Co-authored-by: Ollie Bannister <[email protected]>
Co-authored-by: Ollie Bannister <[email protected]>
| browser.reloadSession(); | ||
| beforeEach(function() { | ||
| originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; | ||
| jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000; |
There was a problem hiding this comment.
sounds good, seeing as these are only being run by GH actions this won't have an impact on any TC builds. Happy for the added test time due to the async behaviour 😄
darcythomas
deleted the
darcy/MC-114/correctly-handle-XHRs-and-virtual-pages-with-Tests
branch
This PR fixes: