Posted by cwgordon7 on June 29, 2008 at 7:11pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | cwgordon7 |
| Status: | needs work |
Issue Summary
This information is from the code coverage report (see http://coverage.cwgordon.com/coverage).
We need to test:
1) tablesort_header() with an existing cell class.
2) tablesort_header() with an existing query string.
3) Actual tablesorting.
Comments
#1
I expect within 3 weeks to test
#2
Opening this up for the testing party.
#3
Tests for 1 and 2
ekes and SeeSchloss
#4
#5
Patch failed to apply. More information can be found at http://testing.drupal.org/node/13893. If you need help with creating patches please look at http://drupal.org/patch/create
#6
#7
#8
Various tabs in various places, coding standards generally needs work on this.
#9
Clean patch.
#10
Miss 3)
#11
Re-rolled, 4 passes, 0 fails, 0 exceptions.
#12
Note that these are unit tests, it would be great to have functional tests too eventually.
#13
Meh. Cross-posted, apologies.
#14
Seems like we could test this whenever we test admin/content/node or similar page.
#15
Sorry, trying to figure out patching, made a little mistake and didn't patch it properly.
#16
Switched status previously based on a bad patch experience. Switching it back to review.
#17
Patched and ran test successfully:
4 passes, 0 fails, 0 exceptions
#18
Since this is a testing party patch, I'm going to be mercilessly nit-picky in this review. It's only because I care. :)
This one was actually hard. :P I had to think about it for awhile.
<?php+ $ts = array('name' => '', 'sort' => 'asc', 'query_string' => '');
?>
a) I realize it's easy enough to deduce that $ts == $tablesort, but it'd be nice to be explicit about that fact. Needs changing throughout.
b) Coding standards dictate each of these on their own lines. However, this is consistent with the way table-related arrays are done so I'm leaving it alone for now.
<?php+ $this->assertEqual($new_cell['class'], 'red active');
?>
Red active? What does that mean? Could you add a message parameter here with more specifics about what's being tested? The general rule of thumb is that by reading the output of the messages in SimpleTest module, we can learn what the test run tested for. This advice applies throughout.
<?php+ $this->assertEqual($new_cell['class'], 'active');
?>
Same here.
<?php+ * Tests the handling of additional query strings by Druapls tablesort API.
?>
What's Druapl? :)
#19
Fixing the critical / pending bugs queues to reflect things which are really bugs or release critical.