Skip to content

Conversation

@mcshaz
Copy link
Contributor

@mcshaz mcshaz commented Apr 23, 2014

this modifies the Example project so that Home/JSUnitTests contains a number of tests. Advantages of including a javascript testing framework are:
-quick multi browser testing
-tests all the included frameworks, so that updating to newer versions of jQuery, jQuery-UI, columnFilters and dataTables can be done a little more safely
-the interaction between the browser,javascript and the MVC framework becomes testable

potential disadvantages are:
-to avoid an error due to jQuery and jQuery-UI being out of sync (on executing jQuery trigger methods), the version of jQuery-UI has been changed to 1.10.3 - this version does not support IE6 (1.9 was the last version to support this, and if this is an important part of the support matrix, I suspect it will work fine with the latest version of jQuery-UI 1.9.?? - the javascript in the tests is ie6 compatible).
-to avoid code rewrite, the example table has been moved to another partial view (reused by the tests). This might make it a tad harder for people trying to follow the example +/- modify it for their own application.
-the test setup fixture (QUnit.testStart) executes multiple handlers, and when this threw an error (before updating the version of jQuery-UI), firefox would freeze rather than failing tests. This was not the case with chrome or IE11. It may be worth wrapping in a try/catch block, but I need to look at the QUnit code first (as I thought QUnit did this).

mcintyre321 added a commit that referenced this pull request Apr 23, 2014
QUnit (javascript testing framework)
@mcintyre321 mcintyre321 merged commit 1f7b7fa into mcintyre321:master Apr 23, 2014
@mcshaz
Copy link
Contributor Author

mcshaz commented Apr 23, 2014

Thanks for merging.

Would you please consider (when you get a chance) stating in the readme for the repository, exactly what you envisage the browser support matrix to be. That way, it is not unreasonable to ask contributors to have changes pass the nunit tests first and subsequently run the qunit test in the stated browsers (eg IE 8&11, chrome and firefox) before submitting a pull request to your repository. It might also be worth forking a dev branch from the main project, and ask contributors fork off this branch.

Thanks again for this really useful bit of software – I can’t quite understand why it hasn’t had more nugget downloads, but hopefully some more stability in the application encourages people to use it more (it has saved me a lot more time than I have spent contributing to it).

From: Harry McIntyre [mailto:notifications@github.com]
Sent: Wednesday, 23 April 2014 9:07 p.m.
To: mcintyre321/mvc.jquery.datatables
Cc: Brent McSharry (ADHB)
Subject: Re: [mvc.jquery.datatables] QUnit (javascript testing framework) (#82)

Merged #82#82.


Reply to this email directly or view it on GitHubhttps://github.com//pull/82.

@mcintyre321
Copy link
Owner

This sounds sensible. I'm not sure about IE8 though - I don't actually have it installed myself.

As for popularity, I'm sure it's down to a lack of promotion, and a lack of documentation. I've considered whether 'documenationware' would be a valid OSS model.

@mcintyre321
Copy link
Owner

Is there any chance you could help me, and take a look at the qunit tests on the upgrade-to-1-10 branch?

I've got the latest version of datatables on there, and the app seems to work but a few of your tests are failing with the new version.

The sort table tests are pretty comprehensible although they seem to be designed for a page size of 10, and it's now pulling back 5. It may be the default has changed, but the example app is working correctly. Not sure about the intent of the other failing test.

@mcshaz
Copy link
Contributor Author

mcshaz commented May 15, 2014

No worries – the number of records should probably be a constant defined at the top of the js file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants