Moved the test selection page improvements out of #250047: Rework the SimpleTest results interface and clean-up of backend code.

This only applies the improvements on that page (mainly adding a "master checkbox" and removing the "run all test" radio button), but shouldn't include any of the other modifications (test results on a different page, changes to batch process, etc).

CommentFileSizeAuthor
#9 select-all-tests1.png11.24 KBfloretan
#9 select-all-tests3.png14.79 KBfloretan
#8 simpletest-check-all.patch10.85 KBboombatower
#6 simpletest-check-all.patch10.83 KBboombatower
#5 simpletest-check-all.patch11.85 KBfloretan
#4 simpletest-check-all.patch11.13 KBboombatower
simpletest-check-all.patch10.16 KBfloretan

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Hm. I'm a little torn here.

On the one hand, yes, this is more consistent with the admin/content/node screen and similar. On the other, you have this checkbox floating in outer space with no context as to what it's for. But of course, that's not your fault, so I made #308468: Text links for 'Select all/None/Invert' a separate bug report.

We also are missing the warning about all tests taking a LONG time. I think this is important. We could stand to improve the help text in general here, maybe best for a follow-up patch.

Sorry, I've officially exhausted all my patch review time today; will take another look at this later in the week.

floretan’s picture

floretan commented

I agree about the floating checkbox, will look at the related issue.

The warning about tests taking a LONG time was really useful when no batch process was used while running tests and we just had to wait, but now that we get an idea of the progress and what is currently running I don't think it's as necessary. When running all tests, I already get an idea after the first few that it's going to take a while, and if I don't want to wait I can just go back to the previous page.

dries’s picture

dries commented

Personally, I think this is a good improvement. I would put the table and button in a fieldset for consistency. Like that, the page would have two fieldsets below another.

boombatower’s picture

boombatower commented
Assigned: Unassigned » boombatower
StatusFileSize
new simpletest-check-all.patch11.13 KB

Addressed #3 which required a bit more clean-up.

floretan’s picture

floretan commented
StatusFileSize
new simpletest-check-all.patch11.85 KB

Same patch from #4, but using the standard tableselect.js instead of hard-coding a "select-all checkbox" and having duplicate code in simpletest.js.

boombatower’s picture

boombatower commented
StatusFileSize
new simpletest-check-all.patch10.83 KB

Re-rolled with a few formatting issues.

Like to see this get in so we can get back to the beast patch.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Why concatenate two strings like this? Long strings which wrap are fine.

+    '#description' => t('Remove tables with the prefix "simpletest" and temporary directories that are left over from' .
+                        'tests that crashed. This is intended for developers when creating tests.'),

Functionally it's a nice improvement, and we should fix the checkbox issue separately. I didn't do a thorough code review, just a quick look.

boombatower’s picture

boombatower commented
Status: Needs work » Needs review
StatusFileSize
new simpletest-check-all.patch10.85 KB

Change to a single line, I still like it the other way. :)

floretan’s picture

floretan commented
StatusFileSize
new select-all-tests3.png14.79 KB
new select-all-tests1.png11.24 KB

To follow webchick's advice (http://www.webchick.net/visualize-your-patches), here's a screenshot. As you can see the position of the checkbox is not an issue anymore (since #5).

Only local images are allowed.
Only local images are allowed.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Thanks boombatower. Did another visual review and nothing jumped out at me, also ran all tests. I'd like to fix up the help page here a bit but that's not in scope for this patch, not to mention it'll be nice to have this in fast to save an extra click.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Needs work

I was going to commit this patch but it looks like it needs a quick re-roll. It currently generates a warning when you go to the test overview page.

warning: call_user_func_array() [function.call-user-func-array]: First argument is expected to be a valid callback, 'theme_simpletest_test_form' was given in /drupal-cvs/includes/theme.inc on line 607.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

@Dries - you need to clear your caches after applying the patch. I also got the error, but it goes away once you've done that.

floretan’s picture

floretan commented
Status: Needs review » Reviewed & tested by the community

The name of the theme function changed from theme_simpletest_test_form to theme_simpletest_test_table, which generates the error mentioned in #11 until the theme registry is cleared. I re-tested the patch and it applies cleanly.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

D'oh! Committed to CVS. Thanks folks!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.