test: slight refactor to support rollup #5601
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
brandonocasey
commented
Nov 16, 2018
brandonocasey
commented
Nov 16, 2018
| * @return {boolean} | ||
| * Whether it is a cross domain request or not. | ||
| */ | ||
| export const isCrossOrigin = function(url) { | ||
| const winLoc = window.location; | ||
| export const isCrossOrigin = function(url, winLoc = window.location) { |
There was a problem hiding this comment.
The tests for this were overriding global/window using proxyquireify which is no longer possible. The only thing I could think to do was add a second argument that defaults to the previous value
brandonocasey
commented
Nov 16, 2018
| @@ -252,24 +245,7 @@ QUnit.test('should expose DOM functions', function(assert) { | |||
| methods.forEach(name => { | |||
| assert.strictEqual(typeof videojs[name], 'function', `function videojs.${name}`); | |||
| assert.strictEqual(typeof Dom[name], 'function', `Dom.${name} function exists`); | |||
|
|
|||
| const oldMethod = Dom[name]; | |||
There was a problem hiding this comment.
This test would have required another override method simliar to the browser module, but it seemed unnecessary. I don't think we needed to check if dom methods were called via deprecated videojs properties. I think its enough just to check that they exist.
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
2 times, most recently
from
62b4b41
to
546a42b
Compare
brandonocasey
changed the title
|
Now contains changes from #5621 |
brandonocasey
changed the title
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
2 times, most recently
from
8e7fea2
to
50cd7b4
Compare
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
2 times, most recently
from
cbe748f
to
1cb0d19
Compare
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
2 times, most recently
from
c5081ee
to
e5b151c
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
4 times, most recently
from
c4e3453
to
8a4b75b
Compare
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
2 times, most recently
from
80d1eca
to
8518638
Compare
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
from
8518638
to
e8f233a
Compare
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
from
e8f233a
to
e47a528
Compare
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
from
e47a528
to
e3bd694
Compare
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
from
e3bd694
to
20fb5f1
Compare
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
from
20fb5f1
to
578d911
Compare
brandonocasey
commented
Aug 29, 2019
| import TestHelpers from './test-helpers.js'; | ||
| import document from 'global/document'; | ||
|
|
||
| QUnit.module('PosterImage', { | ||
| beforeEach() { | ||
| // Store the original background support so we can test different vals | ||
| this.origVal = browser.BACKGROUND_SIZE_SUPPORTED; |
There was a problem hiding this comment.
This isn't used anymore.
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
2 times, most recently
from
8792903
to
5df3d2f
Compare
gkatsev
reviewed
Aug 30, 2019
| this.oldXMLHttpRequest = XHR.XMLHttpRequest; | ||
| this.oldXDomainRequest = XHR.XDomainRequest; | ||
| this.xhr = sinon.useFakeXMLHttpRequest(); | ||
| XHR.XMLHttpRequest = this.xhr; |
There was a problem hiding this comment.
is this not too late to override XHR? That's why previously, we were doing it in the global-shims.js file.
There was a problem hiding this comment.
these tests are broken because the rebase didn't notice a conflict between '@videojs/xhr' and 'xhr' in the import at the top. I think it should be fixed now.
gkatsev
changed the title
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
from
5df3d2f
to
38818f8
Compare
brandonocasey
force-pushed
the
refactor/tests-for-rollup
branch
from
fdd889c
to
b8b5390
Compare
gkatsev
approved these changes
Aug 30, 2019
This was referenced Mar 19, 2020
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.
In order to start building our tests with rollup we have to make various changes, mostly due to the fact that rollup cannot override imports. See the comments below for more information.
Includes changes from #5621