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.

CommentFileSizeAuthor
#20 445950-simpletest-cleanup.patch27.05 KBboombatower
#17 445950-simpletest-cleanup.patch26.6 KBboombatower
#15 445950-simpletest-filter.png11.38 KBboombatower
#13 445950-simpletest-cleanup.patch26.59 KBboombatower
#11 445950-simpletest-cleanup.patch26.46 KBboombatower
#10 445951-simpletest-filter.png3.93 KBboombatower
#7 445950-simpletest-cleanup.patch26.46 KBboombatower
#5 445950-simpletest-cleanup.patch25.48 KBboombatower
#2 445950-simpletest-cleanup.patch25.49 KBboombatower
#1 445950-simpletest-filter.png3.82 KBboombatower
#1 445950-simpletest-cleanup.patch12.98 KBboombatower

Comments

boombatower’s picture

boombatower commented
Status: Active » Needs review
StatusFileSize
new 445950-simpletest-cleanup.patch12.98 KB
new 445950-simpletest-filter.png3.82 KB

Filter interface
filter interface

boombatower’s picture

boombatower commented
StatusFileSize
new 445950-simpletest-cleanup.patch25.49 KB

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

boombatower’s picture

boombatower commented
Title: SimpleTest interface cleanup » SimpleTest interface sepration and cleanup
boombatower’s picture

boombatower commented
Title: SimpleTest interface sepration and cleanup » SimpleTest interface separation and cleanup
boombatower’s picture

boombatower commented
StatusFileSize
new 445950-simpletest-cleanup.patch25.48 KB

Typo, and incorrect array key.

Status: Needs review » Needs work

The last submitted patch failed testing.

boombatower’s picture

boombatower commented
Status: Needs work » Needs review
StatusFileSize
new 445950-simpletest-cleanup.patch26.46 KB

Would be a good idea to update simpletest.test.

dries’s picture

dries commented

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.

yched’s picture

yched commented

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() ?

boombatower’s picture

boombatower commented
StatusFileSize
new 445951-simpletest-filter.png3.93 KB

#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:

'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.

boombatower’s picture

boombatower commented
StatusFileSize
new 445950-simpletest-cleanup.patch26.46 KB

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.

yched’s picture

yched commented

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.

boombatower’s picture

boombatower commented
StatusFileSize
new 445950-simpletest-cleanup.patch26.59 KB

I see...changed.

dries’s picture

dries commented

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! :)

boombatower’s picture

boombatower commented
StatusFileSize
new 445950-simpletest-filter.png11.38 KB

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

boombatower’s picture

boombatower commented

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.

boombatower’s picture

boombatower commented
StatusFileSize
new 445950-simpletest-cleanup.patch26.6 KB

patch would be helpful.

chx’s picture

chx commented
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 :)

yched’s picture

yched commented
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.

boombatower’s picture

boombatower commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 445950-simpletest-cleanup.patch27.05 KB

Good catch.

dries’s picture

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

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

catch’s picture

catch
he/him
Primary language English
commented
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.

catch’s picture

catch
he/him
Primary language English
commented
Title: SimpleTest interface separation and cleanup » HEAD broken: SimpleTest interface separation and cleanup
Category: task » bug
Priority: Normal » Critical
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed that hunk to HEAD. Thanks, catch!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: HEAD broken: SimpleTest interface separation and cleanup » SimpleTest interface separation and cleanup
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Category: bug » task
Priority: Critical » Normal

Sorry. :P Think I'm done now. ;P

boombatower’s picture

boombatower commented

Yea!

Status: Fixed » Closed (fixed)

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