Run our unit tests on different jQuery versions#24749
Conversation
bd57641 to
e6c6914
Compare
There was a problem hiding this comment.
Not sure this makes sense in this branch since we already need Ruby.
I did this in my Hugo branch because the Ruby dep is removed.
There was a problem hiding this comment.
OK, now I read you comment about phantom.
Why do we need phantom anyway? Can't we use jsdom or Chrome/Firefox headless?
There was a problem hiding this comment.
Yep that's a possible switch because phantomjs won't receive any fixes
There was a problem hiding this comment.
Maybe we don't need this new file at all? It's a small snippet so perhaps we could have it in the HTML file?
|
IMO you should revert the http-server switch here; it doesn't make things better since we already need Ruby for Jekyll. Also, the js-tests will probably need to be run sequentially; we can't use the same port simultaneously. |
|
My main issue here is how to launch a npm task in background and lauch our qunit tests 😟 |
b17c962 to
e8d0378
Compare
|
I added Karma to run our test on ChromeHeadless it's possible to add FirefoxHeadless too so if it's wanted do not hesitate 👍 With this implementation we can run our unit tests on jQuery 1.9.1 and the latest release of jQuery 3 |
| }, | ||
| afterEach: function () { | ||
| $('.modal-backdrop, #modal-test').remove() | ||
| $(document.body).removeClass('modal-open') |
There was a problem hiding this comment.
I added that because in our index.html which run our unit test we did the same see : https://github.com/twbs/bootstrap/blob/v4-dev/js/tests/index.html#L82
There was a problem hiding this comment.
But do we need to do it in modal.js if we do it in index.html?
There was a problem hiding this comment.
Karma do not use our index.html it generate a new index.html
fd23f0d to
95fdf6a
Compare
| jQueryVersion, | ||
| jqueryFile, | ||
| 'assets/js/vendor/popper.min.js', | ||
| 'js/dist/util.js', |
There was a problem hiding this comment.
Would it work if we did js/dist*.js and just excluded index.js? Or even just include everything?
There was a problem hiding this comment.
Nope because we have to be sure util.js is the first included script
|
I made a test using |
6bac084 to
a1b850b
Compare
|
@XhmikosR I changed the reporter, and now we use wildcard to include our dist files and added Firefox launcher |
| @@ -0,0 +1,39 @@ | |||
| module.exports = function (config) { | |||
| const jqueryFile = process.env.USE_OLD_JQUERY === 'true' ? 'js/tests/vendor/jquery-1.9.1.min.js' : 'assets/js/vendor/jquery-slim.min.js' | |||
There was a problem hiding this comment.
Maybe process.env.USE_OLD_JQUERY would be enough?
There was a problem hiding this comment.
I don't understand what you mean here 😟
There was a problem hiding this comment.
I was wondering, wouldn't this be enough?
const jqueryFile = process.env.USE_OLD_JQUERY ? 'js/tests/vendor/jquery-1.9.1.min.js' : 'assets/js/vendor/jquery-slim.min.js'Oh, and perhaps we should only change the filename and keep the path since it's common?
4cf67ea to
1c88c55
Compare
ecfc5a8 to
5cbf19f
Compare
5cbf19f to
45b5107
Compare
|
Do you have any concerns about that PR @mdo ? Or can we merge ? |
|
About Popper.js on jsdom, the suggested way is to mock Popper.js away directly since it can't work on a jsdom environment even if you polyfill everything that's required. |
|
Seems better in parallel @XhmikosR 👍 |
fe9f627 to
069ab73
Compare
069ab73 to
27e0129
Compare
I added a new devDeps becausephantomjsdo not allow to pass parameters in URLBTW if this PR is accepted I'll changejs-test-cloudto use ourservertask 👍Add Karma to run our unit tests on ChromeHeadless instead of PhantomJS + it's allow us to run our unit tests on two jQuery versions
Fixes: #24705