SimpleTest interface separation and cleanup

Posted by boombatower on April 27, 2009 at 8:37am

Issue Summary

A while back I worked on a large patch that cleaned up the code under the hood and some interface elements, #250047: Rework the SimpleTest results interface and clean-up of backend code.

The code died as I kept having to split it up and split it up due to original size. The issue has basically died. I started over and just focused on the interface and backend code (ignoring the rewrite of results display). All of which should be done in the same patch.

Items accomplished:

Separate test selection form from results display (both interface and code).
Get rid of $_SESSION['test_id'].
Allow the test_id to be specified when viewing results, thus making it possible to display older test results that may be left by setting simpletest_clear_results to false.
Clean up results display a bit.
Add a filter to provide a "smarter" re-run interface.
Begin seperating interface code from SimpleTest API code. Eventually I would like run-tests.sh and the web runner to share test running API wither there respective interfaces completely separate from the running code.
General clean-up of test selection form and results display code. (You can actually read it!!)
DBTNG results query and provide API function.

I plan to work on further clean-up patches to truly desperate out the interface and API and eventually integrate the API with run-tests.sh.

Comments

#1

Posted by boombatower on April 27, 2009 at 8:41am

Status:

active

» needs review

Filter interface

Attachment	Size	Status	Test result	Operations
445950-simpletest-filter.png	3.82 KB	Ignored: Check issue status.	None	None
445950-simpletest-cleanup.patch	12.98 KB	Idle	Passed: 11117 passes, 0 fails, 0 exceptions	View details

#2

Posted by boombatower on April 27, 2009 at 8:46am

I seem to have rolled patch during some interesting CVS changes...

Attachment	Size	Status	Test result	Operations
445950-simpletest-cleanup.patch	25.49 KB	Idle	Failed: 11100 passes, 19 fails, 8 exceptions	View details

#3

Posted by boombatower on April 27, 2009 at 8:55am

Title:

SimpleTest interface cleanup

» SimpleTest interface sepration and cleanup

#4

Posted by boombatower on April 27, 2009 at 8:55am

Title:

SimpleTest interface sepration and cleanup

» SimpleTest interface separation and cleanup

#5

Posted by boombatower on April 27, 2009 at 9:02am

Typo, and incorrect array key.

Attachment	Size	Status	Test result	Operations
445950-simpletest-cleanup.patch	25.48 KB	Idle	Failed: 10914 passes, 17 fails, 32 exceptions	View details

#6

Posted by System Message on April 27, 2009 at 9:21am

Status:

needs review

» needs work

The last submitted patch failed testing.

#7

Posted by boombatower on April 27, 2009 at 4:48pm

Status:

needs work

» needs review

Would be a good idea to update simpletest.test.

Attachment	Size	Status	Test result	Operations
445950-simpletest-cleanup.patch	26.46 KB	Idle	Passed: 11117 passes, 0 fails, 0 exceptions	View details

#8

Posted by Dries on April 29, 2009 at 7:49am

The filter interface confuses me, to be honest -- and I'm an expert user running the tests x times a day. I think the labels need work. Maybe something like: "Re-run the following tests"? It is more verbose but it is much clearer.

#9

Posted by yched on April 29, 2009 at 11:39am

Minor: maybe it's intentional, but it seems you should change the $batch['redirect'] property in simpletest_run_tests() rather than adding a drupal_goto() in _simpletest_batch_finished() ?

#10

Posted by boombatower on April 29, 2009 at 8:04pm

#8: The filter is used to "filter" the selected tests to re-run. So if you select "Fail" then it runs all the selected tests that failed, or "Pass" passed, etc.

How about just "Re-run" as the other text is rather long.

filter

#9: adding:

<?php
'redirect' => 'admin/development/testing/results/' . $test_id,
?>

in simpletest_run_tests() does not work and there is no $batch variable to reference in _simpletest_batch_finished(). I am not sure what you are referring to.

Attachment	Size	Status	Test result	Operations
445951-simpletest-filter.png	3.93 KB	Ignored: Check issue status.	None	None

#11

Posted by boombatower on April 29, 2009 at 9:21pm

How about leave "Filter" but change the button text to "Run tests" link on the previous screen.

I think most people, especially developers, will know what "filter" means and with "Run tests" (same as on other screen) there should be enough to clarify what you are filtering.

Attachment	Size	Status	Test result	Operations
445950-simpletest-cleanup.patch	26.46 KB	Idle	Passed: 11117 passes, 0 fails, 0 exceptions	View details

#12

Posted by yched on April 29, 2009 at 10:02pm

Hm sorry, I got confused by that $batch['redirect'] property set in simpletest_run_tests(). It is indeed an internal property, written in batch_process().
Usually the $form_state['redirect'] property is used to determine the batch redirect page. Since simpletest skips FAPI's automatic batch_process(), the redirect url has to be passed as the 1st param of the explicit batch_process() call in simpletest_run_tests(). The line that sets 'redirect' can go away.

#13

Posted by boombatower on April 30, 2009 at 1:58am

I see...changed.

Attachment	Size	Status	Test result	Operations
445950-simpletest-cleanup.patch	26.59 KB	Idle	Passed: 11117 passes, 0 fails, 0 exceptions	View details

#14

Posted by Dries on April 30, 2009 at 2:50pm

This patch looks good to me -- although it is hard to review when you move things to a new file. If other people thing the labels are good, I'll commit this. Should be ready! :)

#15

Posted by boombatower on April 30, 2009 at 8:25pm

To make it easier for people to review the labels on the interface I have included a full screenshot of the test results page.

results

Attachment	Size	Status	Test result	Operations
445950-simpletest-filter.png	11.38 KB	Ignored: Check issue status.	None	None

#16

Posted by boombatower on May 1, 2009 at 12:55am

Moved actions above results upon the suggestion from chx. When failed results are displayed it will be hard to find the actions as confirmed by Fitts's law.

#17

Posted by boombatower on May 1, 2009 at 12:55am

patch would be helpful.

Attachment	Size	Status	Test result	Operations
445950-simpletest-cleanup.patch	26.6 KB	Idle	Passed: 11117 passes, 0 fails, 0 exceptions	View details

#18

Posted by chx on May 1, 2009 at 12:59am

Status:

needs review

» reviewed & tested by the community

Yup. The top of the screen is much easier to find :) So we want actions there. I would say it's fairly uncommon to run tests once anyways so even for passed tests it's more likely we want to run again. Rerunning failed tests only is a dream :)

#19

Posted by yched on May 1, 2009 at 1:08am

Status:

reviewed & tested by the community

» needs work

Sorry for being a painful nitpicker, but the 'redirect' => 'admin/development/testing', line in simpletest_run_tests() should go.

#20

Posted by boombatower on May 1, 2009 at 1:17am

Status:

needs work

» reviewed & tested by the community

Good catch.

Attachment	Size	Status	Test result	Operations
445950-simpletest-cleanup.patch	27.05 KB	Idle	Passed: 11117 passes, 0 fails, 0 exceptions	View details

#21

Posted by Dries on May 1, 2009 at 11:02am

Status:

reviewed & tested by the community

» fixed

Thanks -- let's go with this and refine from there. Committed.

#22

Posted by catch on May 1, 2009 at 12:34pm

Status:

fixed

» reviewed & tested by the community

Test bot is broken. Looks like simpletest.page.inc didn't get committed along with the rest of the patch.

#23

Posted by catch on May 1, 2009 at 1:24pm

Title:	SimpleTest interface separation and cleanup	» HEAD broken: SimpleTest interface separation and cleanup
Category:	task	» bug report
Priority:	normal	» critical