Rework the SimpleTest results interface and clean-up of backend code

I reviewed the JavaScript code over IRC with cwgordon7 and posted the revised version of the main function.

I guess the reviewed Javascript needs to be integrated and the patch rerolled?

I'm not sure the filtering should happen on a per-group basis. I'd be inclined to make it a global toggle.

In general, I find the output to be quite messy. I'm not, at all, a big fan of the nested fieldsets. I find it very tedious to open and close them. I'd much rather have a flat page or single-level fieldsets with global filter options.

All in all, I think we're on the right track with this filter mechanism, but I'm not convinced we should sprinkle every fieldset with it.

On a related not, it is not clear why we put square brackets around the category names.

@Dries: Agree with the brackets. All that was added at the sprint, I'm not sure who added it either. :)

@cwgordon7: Wasn't trying to be condescending. When I was referring to adding I was talking about how it was separated out of message.

Gordon, Jimmy -- looks like a solid plan! Great.

I think there are outstanding issues raised by Dries. Correct me if that is wrong.

I tidied up the JavaScript a bit, so here's a rerolled patch - it's almost exactly like the previous one, simpletest_message_filtering_05.patch, with the only difference being the JavaScript.

I think this is starting to look good.

I'm confused though, what is 'Filter by category'? From the looks, I'd expect this to match any pattern. From the text, it sounds like it would only match categories. Is there a reason we don't match everything?

I'd like to see us try and improve the usability a bit. As I said, I was reasonably confused by the label of the form. I'd recommend that we simply match everything.

generally looks good.

however, Passes should be deselected by default

Also, a Group that has all passes should still show the basic info even when Passes is deselected, e.g.:

Book
1 test case complete:
97 passes, 0 fails and 0 exceptions.

I think having the pass output like that is important - otherwise people will think some tests didn't run at all (and with no failures you'd get no feedback whatsoever). Reassuring.

Couple changes I think would be nice, otherwise looking nice:

• Adding a fieldset around the filter options. That would be cleaner looking and help separate them from the results. Also I think it should be collapsed by default since many times looking at all the assertions is what is useful.
• Can the event on the filter input field be changed from after every character typed to after a user stops typing (I know that isn't an event, but not sure how difficult it would be to implement a delay after last typed character?) or onblur or capture enter key press. As it is currently it is very slow when very many assertions are displayed on the page and it attempts to sort after every keystroke.
• The simpletest.test needs to be updated since the [ ] were removed from the assertion types. (+1 for that change)
• Having the default assertion type be set to the test name is somewhat long and odd. I think a better solution would be to set it to "Test" or something since the assertions are already in a fieldset with the tests name would be better.

This patch changes simpletest.test to work with new patch except for:

$this->assertAssertion($this->pass, 'Other', 'Pass');
$this->assertAssertion($this->fail, 'Other', 'Fail');

the "Other" types since that is being discussed and not yet confirmed either way.

* I think we should get rid of all the green status messages. I know, that's a separate issue, or maybe it isn't as it could be Javascripted out.

* I like the 'Run more tests' link. It cleans up the UI and let me focus on the test results of this run.

* The filters don't seem to work though. It doesn't show the passed tests ... See screenshot.

While we are at it, I would really like a "Rerun the same tests" button on the result page.

@Damien Tournoud: That would be great.

@Dries: Are you talking about the directory created messages?

Based on #31 I believe this needs work, and #32 would be nice to incorporate.

Regarding the directory created messages, there's a generic patch for those, changing the dsm to a watchdog (which I'd support): #257537: file_check_directory is verbose on success. - Might as well fix across the board as special case it for tests.

The directory issue hs been committed elsewhere. Just need to addres Dries' other points and Damien's as well.

I much prefer the tables, but I like the bolding of non-framework assertions.

The re-run tests button is good, and I like that the entire list of tests isn't display, but a link to them instead.

I would strongly vote for tables to remain.

As long as this patch continues to provide a re-run tests button #286946: Remember the tests that were selected on the test results page is a duplicate.

Going into this not having read the rest of the issue, nor reviewed the patch (will in a moment) but only clicked on the screenshot @#38 (this simulating a typical end-user), here are my thoughts:

• Site title matches is bold. Ahhh! Scary! Does that mean it REALLY passed? Does it mean it failed? But if it failed, why is it green and not red?
• Also, why does it list a bunch of sub-things related to it, which other tests don't?
• Why is "Found assertion" such a big deal that it's bolded too?
• Where do I find fails/exceptions in a huge bulleted list like this? Will they be grouped together at the top? Interspersed between?

Reviewing the patch now...

Er.

When I run this patch, check off a test and then click the "Run tests" button, I get a blank page with just the word 'HERE'.

Hm. After removing + print "HERE"; from the patch, the results page still doesn't show me any results. I tested with Filter's tests, which have failures at the moment.

They're ugly, they overflow off to the right of my screen, they're a rainbow of too-bright colors, the tableheader is annoying, and it makes me scroll down way too far.

The same colors are used in new patch...and the same number of assertions are displayed with no way to collapse them....so how is the scrolling an issue?

As for the crashing when parsing the same number of assertions are displaying with new method...very similar data to parse. Could be related to system running it? I don't ever have firefox crash, but it does take a few moments longer to parse, but isn't that to be expected?

Table is a good format for displaying large amounts of data, columns give you a lot of power. As seen in similar applications throughout the computer world I think a table is the best choice. If the main argument against it is cosmetics that is not really a big deal, especially since bulleted list isn't any better (opinion). I think testing is mainly concerned with results (data) and table displays the data in an organized format.

Still not seeing all this overflow and such with tables. :) The main change appears to be the removal of much of the debug information such as file, line number, etc. Things that I think are very important.

Hrm.

1. I was just about to post that "No, it's still broken cos it's only showing me the number of test passes/fails" before I accidentally clicked on "Filter: 163 passes, 12 fails, and 0 exceptions" and saw that it was expandable. So we need some sort of UI thing to indicate "this will expand when you click it and there will be more stuff below it" since right now it looks like just any old bulleted list item. Maybe a "+" icon, or a ">" icon like we have on menu items?

2. I get that you're trying to compress the output, but it's annoying that there were test failures and I had to click twice to see what they were. I would prefer if test failures were auto-expanded so that I could immediately see them in front of me. This would get real tedious if I was truly doing test-driven development which would involve re-loading this page 50 times trying to figure out where my tests were still failing.

3. I still don't know why some of these are bolded and some aren't. It looks like a bug. I have a feeling it's on purpose, but if it is, it's not clear why "Parse simple www-string but not the end-of-sentence period." and below is bold, but "GET to http://localhost/head/node/1, response is 7660 bytes." and above is not. Are the bold ones assertions from within filter.test itself? If so, we should probably preface each line with some sort of indicator, like what filename the assertion was found in?

4. When I click away from this page and go do other stuff, then come back to the testing page because I want to test, it shows me the collapsed test results rather than the test selection form. That's very odd, and not what I would expect. We should clear the cookie or whatever's maintaining state if admin/build/testing is hit directly.

5. I love love love the "Re-run same tests" button. :D

6. When I unchecked the "Fails" button, suddenly the entire output of this page went away. That's not what I would expect. I would expect the "Filter: 163 passes, 12 fails, and 0 exceptions" and the "Core filters: 68 passes, 12 fails, and 0 exceptions" to stay there, but for the individual failure rows to disappear, so that only the passes remain. OTOH, when I uncheck "Passes" the page does do what I expect, event though I realize it's taken "Filter administration functionality: 95 passes, 0 fails, and 0 exceptions" out entirely. I guess the logic would be, "If unchecking one of the options results in an empty list, remove its parent. Otherwise, keep the parent in place, with the unchecked children removed."

I've tried to annotate a screenshot so this feedback is a bit more clear.

I'll try and be frank. I'm not trying to start a war, but from the screenshot (haven't gotten around to running patch) there is no line number or file. Not sure, but I would prefer if my comments weren't taken as an offense, as they seem to be. There are several things that I really like about the patch as I have mentioned above.

I'll take a look at the patch in more detail in a bit.

Oh, sheesh. Again, I didn't realize those sub-items were clickable and revealed more data. We really need a different UI construct. :\

One thing about the patch: I'm now sure why exactly there are two functions, Drupal.behaviors.simpleTestResultsCollapse and Drupal.batchUpdate.simpleTestResultsCollapse, with exactly the same code.

If the same function needs to be under two namespaces, you can simply to this:

Drupal.behaviors.simpleTestResultsCollapse = Drupal.batchUpdate.simpleTestResultsCollapse = function() {
  $('li > span.simpletest-overview').click(function() {
    $(this).siblings().children('ul').toggle().toggleClass('simpletest-hidden');
  }).each(function() {
    $(this).siblings().children('ul').hide().addClass('simpletest-hidden');
  });
};

As an added bonus, I chained .each with .click. As far as I know, chaining isn't just pretty syntactic sugar; it should also reduce perf hits because you don't select the same set of elements twice or more times. (The last time I checked, jQuery doesn't cache recent element selections.)

I apologize for not being able to reroll this patch myself. I'm currently packing up for Canada, and packing mes bagages is a pain :(
Whistler should be fun, though.

Subscribing.

I agree that the file names and line numbers should be hidden by default. That annoys me endlessly in simpletest for D6. On a sidenote, I think we also need a better interface for selecting tests. At least if the D7 UI is the same as D6's. I have to scroll way too much.

I'll try to find the time for a more in-depth review later.

subscribing

@Wim Leers: not the same interface for selecting tests. I felt the same way and change it a while back.

@cwgordon7: Ok the clicking is much more reasonable. Sorry about that the whole misunderstanding grew out of the screenshot. I agree with webchick that the interface isn't easy to figure out though.

#58:
1) When the test is running it provides links which interrupt the tests and cause them to fail when clicked. (intended?)
2) Nothing shows on the results page. Even when I check "Passes"

1) On the page doing the batch process the results now form links, but when clicked they take me to the results page, which has problems as in #2, and the tests never finish running.
2) Page displays with checkboxes and (from picture) intended interface, but not assertions display. (cache issue with javascript maybe?)

I tried several different devious ways to screw up SimpleTest and could not reproduce the items highlighted by Jimmy, either.

I can confirm that this new version addresses all of my previous concerns. A couple other things I found during this pass through:

1. The contrast between the green and red is poor, and I need to squint to tell the difference. The #2 Web Content Accessibility Guidelie is to NOT use colour as the only way of communicating information, for a variety of reasons. I suggested appending (pass) (FAIL) and (EXCEPTION) to the test results to meet this requirement.

2. There's a "check all" box at the *bottom* of the form, which I gather is for convenience, but breaks core standards. I'd remove it, leaving the one at the top like we have on admin/content/node and similar.

3. One kind of annoying bug is if you expand out the test results during the test run, the next test group that runs will sproing the thing back to the default collapsed view. But Charlie's informed me this requires changes to progress.js to fix, which is out of scope for this patch. Would be awesome to get it in later though... and maybe even the checkboxes on this screen, too! :)

Ok, that addresses all of my major points.

Tentatively marking RTBC.

I'd like to take a look at it. Will do in a bit.

CNR for Jimmy. :)

I think the interface is much improved, but there a few small/trivial items that are inconsistent or I think should be changed.

• "Re-run same tests" -> "Re-run tests" as "same" is somewhat redundant. - changed
• Status of assertion should be more prominent. As in previous table layout it was easy to scan column and see the assertion's status. I moved it to the front of message. -changed
• Consistent case on status. The pass was lower and the other two messages (fail, exception) were uppercase. That is somewhat inconsistent. If you are trying to emphasize that should be done in a different manor I think. Since you can also filter on the status it isn't necessary to make the non-pass messages different. - changed

The code has a number of very long lines of code that are hard to read. Also some places where the same condition is executed 5 times in the same set of code.

Specifically this line of code was written 5 times and uses embedded conditions in the shorthand format. Not easy to read.

($results[$class]['#fail'] ? 'simpletest-fail expanded' : ($results[$class]['#exception'] ? 'simpletest-exception expanded' : 'simpletest-pass collapsed'))

I rewrote code to assign the result to a variable and use a switch statement to evaluate the condition. Fewer lines isn't always better, I think readability is more important.

Same issue with exception status. The extra long lines with embedded conditions are in a number of places and it would be preferable if the code could be simplified for readability.

In simpletest.test the function assertAssertion($message, $type) lost the $status attribute which somewhat removes the purpose of the test. The status message is displayed and could be parsed as before. (I'll leave that to you)

When the filter has removed all possible assertion from an item it would be preferable if an empty message displayed.

I have yet to review other areas, but I think these need to be addressed first. This is a major change and I think the code should be reviewed thoroughly.

Since this seems to have become neglected I've decided to finish it up.

First off is a re-roll which I am currently working on.

After a bit of wrangling and cleanup I have a re-roll.

I will be checking it over again, run all tests against it, and post conclusions. In the mean time reviews are welcome.

Issue that seems to be present is the file display in not always correct.

For instance from the block test:

[PASS] Box successfully created.

    * Group: Block functionality
    * File: drupal_web_test_case.php
    * Function: testBox
    * Line: 285

The function is correct, but the line and file refer to the assertion call not the actual function.

Found a fix for that problem, was issue with backtrace traversing logic.

Working on removing some interested "debug" output.

Important changes (doesn't include general clean-up items):

• Fixed backtrace traversing.
• Remove custom_caller due to improved backtrace traversing.
• Change 'caller' field to 'function' in db for consistency with use.
• Lots of additional comments.

Working on further clean-up or related interface code.

Large number of changes locally, I have to go now, but I will patch later.

So don't make new patches wait for mine which has a large number of patches.

Important changes (doesn't include general clean-up items):

• Re-moved rerun tests dependency on db results. It shouldn't be necessary and results no longer exists due to other patch.
• Improved re-run tests to handle more then one test class.
• Split up results and select tests code making it much much cleaner and easier to read.
• Removed lots of left over code.
• More comment goodness. (Lots!)

A few minor changes, I think this one is ready for thorough review.

Hmm...I think I fixed the traceback logic so that it works properly and doesn't need that anymore.

If anyone sees any problems let me know.

still applies with offsets.

Applied the patch, reinstalled the environment.

Ran tests without any issue. The new interface is neat.

But: the checkbox on top of the test selection page has no apparent effect, and the "Re-run tests" button has no effect (it returns to the test selection page).

Two feature requests:

* I hate having to click to expand a test group when I selected only one test group. Could we make a special case for this.
* The expand/collapse status is lost when the results are refreshed during the test.

I guess this is a small CNW.

"But: the checkbox on top of the test selection page has no apparent effect, and the "Re-run tests" button has no effect (it returns to the test selection page)."

May need to clear your browser cache as the javascript files may not be updated, checkbox works for me.
Re-run tests works great for me...hmmm.

"I hate having to click to expand a test group when I selected only one test group. Could we make a special case for this." - not sure what you mean by that. (possibly follow up issue)
"The expand/collapse status is lost when the results are refreshed during the test." - is an issue to be solved later.

Had the clear test results code commented out.

The new UI looks like a great improvement, unfortunately, I'm struggling with my test environment. See #304328: SimpleTest requires max_execution_time > 30 seconds.

My one comment is that people might not understand 'Clean environment'. I know I didn't when I first saw that button. It might be better to make that a 'Clean environment' checkbox with a little bit more explanation. By default, it should be checked.

Clean environment is done everytime the tests are run. The button is intended for use when the tests crash and you need to manually clean the environment.

The button used to have a fieldset around it explaining its use. As of the patch I started work on the fieldset was removed and the description was placed at the top of the page.

Should I move it back to the fieldset way?

Could we move 'clean environment' to cron instead of having it in the ui?

Let me clarify its use.

The equivalent of clean environment is run after the test suite has been run. So everytime you select some tests to run, when they finish the environment should be clean.

If the tests crash, usually when in development, the environment may not be cleaned up. So it is really a developer tool, but I guess SimpleTest itself is. The button just makes it easier to clean up the mess, since otherwise you have to manually do it all, which gets old really fast.

It appears that this patch causes certain tests to timeout. Like "Site-wide contact form." and possibly "drupal_http_request" for starters. Which may be [304328] and webchicks issue on #296310: TestingParty08: drupal_http_request redirects need a test

It appears that the contact one works by itself, just not in the batch I ran it with..hmm

But drupal_http_request dies with:

PHP Notice: Undefined variable: function in /home/jimmy/software/php/drupal-7/modules/simpletest/drupal_web_test_case.php on line 105, referer: http://drupal-7.dev.loc/batch?op=start&id=136
error 	PHP Fatal error: Uncaught exception 'PDOException' with message 'INSERT INTO {simpletest} (test_id, test_class, status, message, function, line, file, message_group) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7) - \nArray\n(\n [:db_insert_placeholder_0] => 7\n [:db_insert_placeholder_1] => DrupalHTTPRequestTestCase\n [:db_insert_placeholder_2] => exception\n [:db_insert_placeholder_3] => Undefined property: stdClass::$code\n [:db_insert_placeholder_4] => \n [:db_insert_placeholder_5] => \n [:db_insert_placeholder_6] => \n [:db_insert_placeholder_7] => Notice\n)\nSQLSTATE[23000]: Integrity constraint violation: 1048 Column 'function' cannot be null' in /home/jimmy/software/php/drupal-7/includes/database/database.inc:365\nStack trace:\n#0 /home/jimmy/software/php/drupal-7/includes/database/mysql/query.inc(29): DatabaseConnection->query('INSERT INTO in /home/jimmy/software/php/drupal-7/includes/database/database.inc on line 365, referer: http://drupal-7.dev.loc/batch?op=start&id=136

Appears to be issue with getAssertionCall...working on it.

This patch adds:

$trace['function'] != 'error'

to the trace crawler.

Otherwise it gets mad when notices and stuff are recorded.

Getting notices, which is what caused issue:

[EXCEPTION] Undefined property: stdClass::$code

    * Group: Notice
    * File: drupal_web_test_case.php
    * Function: error
    * Line: 333

boombatower, I know cleanup is only for if tests abort, I still think it could be done on cron though. I'd be fine with that happening in a followup patch, but it just occurred to me it'd be nicer to remove it altogether than worry about help texts.

It's not 100% clear. So, will running some test automatically clear all my previous environments or just the last one? Personally, I don't want to worry about these questions. Thanks for answering.

Basically each run of the test suite cleans up after itself. If the literal clean environments code was run that it might interfere with a different batch running at the same time...something that would be used on testing slaves for t.d.o.

So clean environments is used for cleaning up left overs when tests crash during the development cycle.

What text would make it more clear? I would prefer to get other feedback since you have an outside perspective, not having written the code.

Makes sense that we'd want to avoid running clean environment when another test might be running, so cron is no good. How about moving it to devel.module though?

Cron to devel module? sure. The button, I would prefer is stays in core. Just may need better explanation which I can add.

No I meant move the button to devel module, I personally wipe my HEAD database every few days anyway, which is my main use for simpletests, if I was using simpletest for a site, I'd also be using devel, so I don't think we need it there.

What about reverting to the old interface on that matter? The fieldset:

Clean test environment

Remove tables with the prefix "simpletest" and temporary directories that are left over from tests that crashed.

... makes complete sense to me. We could simply add that this should not be done while other tests are running.

That's fine with me too, and it's a very small issue in an otherwise lovely patch. Marking to needs work for that bit.

#102: That is what I suggesting.

Added the fieldset back with a bit more description as to its intent.

It would be nice if the tests that failed still had their checkboxes toggled on so that you could just click "Run tests" to re-run all the previously failed tests.

Hmm...that would be interesting. How about we make a separate issue for that as this patch is already very large and holding up several other issues. Possibly a button that says re-run failed tests...or a combo with pass/fail/both and then a re-run button?

If someone could run the test suite (I'll hopefully get around to it) and compare the results (as there appears to be fails/warnings in core last I checked).

Ok, I'm worried about this patch: it has became huge and hard to review, and it mixes rework of backend code (that we need) and of the UI (that could wait).

About the backend: for another patch (#304924: Extend drupal_error_handler to manage exceptions), I need to keep the $custom_caller facility.

About the UI: the progress report is completely unusable on my laptop. Firefox hangs for several seconds each time a test ends, so I can't do anything else in the browser while I run tests. I think we should fallback to the old progress report. On the other hand, I really like the new display of test results. It is very pretty and usable.

Could we split the work on the two, produce a patch for the backend code in a separate issue to get this committed as quickly as possible?

Based on #107, I submitted separately a backend cleaning patch: #305077: Rework simpletest backend.

Boombatower at #106: sounds good! #305150: Keep running tests checked

#107: We can look at the changes made by this patch, but while cleaning up the backend it is very easy to cleanup UI along with it. Especially since new UI needs new backend code anyway. Separating the two will only create more work and postpone this patch.

As for the custom caller stuff that might need to come back, I removed it since it isn't used anywhere and if the code works properly I don't see a use for it.

Will re-roll upon #305077: Rework simpletest backend getting committed.

Ok, #305077: Rework simpletest backend is in. Back to code needs work.

I'll try and get to this tomorrow. I have class all day, but should have plenty of time after that.

This is proving to be a lot of fun, which was one of my reasons for not splitting this as they change much of the same code. Hopefully I can get this back together tonight.

Unfortunately, huge patches that change tons of things are too hard to review effectively, so they sit there forever until someone has a solid 1-2 hour block of free time to give it a thorough review. Even the backend one was pushing it, in terms of size and complexity.

Splitting patches up allows us to get them to RTBC in a much more timely manner, which results in more patches being committed faster. Sorry about the re-roll, but I honestly think this general approach is best.

I believe this is a merge, although there were so many conflicting sections and code that was removed in same areas as this patch that I am by no means sure.

There are issues with simpletest.test, as that doesn't appear to have been completely merged, and simpletest_rerun_form_submit never gets called so re-run button doesn't work. So far I can't figure that one out. I've added

drupal_set_message('called simpletest_rerun_form_submit().');

If you see it then it or the batch API loads and it re-runs tests let me know.

Marking as review to get more eyes looking at merge and two issues described above.

Some of the changes to simpletest.test where made before I got involved in patch and remove testing functionality, so not all are good. I would recommend just using the patch as a guide to fix issues if you attempt it.

After allot of work and messing around I eventually got simpletest.test passing again!!

Talking with chx and others the following items are to be added to this patch (some already partially implemented)

• re-check failed tests in some manner
• batch api page summary

I'm going to play with these a bit, but the re-run tests form still doesn't work.

Almost done with it, but I have to go out.

This patch includes everything except the batch API summary, but I would like to get some reviews and maybe commit before adding more.

Patch applied and ran some test. I like the results as they're going along. The first time I ran the tests passed and it jumped nearly right away back to the tests selection. I didn't get any chance to review results. Is this intended? I tried running a test I modified to fail, and it did the same thing, once the tests were done, it kicked me back to admin/build/testing.

Check all is much nicer than 'run all tests', and much more consistent with core in general. Also makes it easy to run all tests but one etc.

Reporting gets reset on batchapi (which I assume is the TODO), but frankly, who cares. Ought to be a followup patch.
However, I'm getting 'unresponsive script' timeouts on the update (complex) db layer tests, which I don't get without the patch, and firefox is nearly crashing, so the js might be a bit heavy here. That'd be a real issue for me in actually running all tests.

Not sure why [PASS] etc. is in bold - seems unnecessary.

I can't run all tests successfully with this patch for the reasons mentioned above, I can without. So very sadly, needs work.

@Dave Reid: Make sure you re-install SimpletTest/refresh cache.

@catch: "The assertions listed in bold text are those made by the tests themselves, rather than the implicit framework tests."

It should only bold the assertions made by a test as opposed to those made by the testing framework.

This should fix the issue of not running the tests. I have reverted to the previous display mechanism.

Add batch API summary line.
Changed order that test result lines are output, so newest show at top. This is useful when running all tests so you don't have to scroll down to see.

This should conclude the changes made by this patch. It does a TON of things. Anything else lets try to make a separate issue.

There are a few other ideas floating about, but this patch just need to be reviewed and committed.

All tests pass:
Overall results: 5444 passes, 0 fails, and 0 exceptions

but sadly it kills the browser when trying to display them....we have some scalability issues.

Perhaps a pager?

This patch is the perfect occasion to use the word ginormous.

I know what's left is all related stuff, but can't we break it up in more pieces?

Here's a possible separation:
- add a "master checkbox" to test overview page.
- changes to batch processing.
- move test results to another page.
- filtering of test results.
- #308262: clean-up and documentation of simpletest.js
- #308259: contact.test cleanup

I already took care of the easiest ones (here's the patch from #126 with the rest). Let's tackle the others, doing these one by one will make it much easier to have experts looking at each one individually.

Also moved the "master checkbox" and other changes to the test selection page to #308272: Improve test selection page.

Also, #308285: Attach javascript callback to batch processes.

Only two items from the list on #129 still need to be broken out:
- move test results to another page.
- filtering of test results.

However, the first one depends on #308272: Improve test selection page and #308285: Attach javascript callback to batch processes, and the second one depends on the first one, so let's get these committed first.

If two patches are that related....is it really advantageous to split them up? Would seem they should be part of the same patch....

I'm having a hard time making progress on this with the multiple split ups of this patch. Everytime I get it put back together after people make changes when they split off chunks someone else seems to split it up again.

I have 1 "bug" need to add pager.....please don't make that hard to add.

All we've done is make it so that I have to apply 6!?!??! patches to get the whole picture...and split it back up again....

Getting...rather annoying.

I don't see this as constructive. The batch API js and contact cleanup are the only two I'm seeing as out of the scope, although even the contact cleanup is directly due to the new UI changes...

When I issue another version I will leave those two out, but I'm not wasting my time with 4 other patches...come on.

@boombatower: I realize that this split-up is making it a little more difficult to see the big picture, and I'm not sure we really need to split up what's left into further pieces.

However, by separating this big patch into smaller chunks, the two cleanup patches are already RTBC. The batch API changes were very easy to separate, but looking at it as a standalone patch makes it a lot clearer what is happening (think of the commit message). That's also four less files modified by this patch.

#308272: Improve test selection page wasn't as easy to split off, and this one could have stayed here, but this way it's also easier to get it fully reviewed and committed.

@boombatower: Sorry, but I agree with flobruit. Please see http://webchick.net/please-stop-eating-baby-kittens.

#308262: clean-up and documentation of simpletest.js is the perfect example of a patch that, once broken out, was a) able to be reviewed and committed much more quickly, and b) when viewed in isolation from the rest of the extensive changes, resulted in additional fixes.

This needs to be re-rolled and "merged" with the split off parts.

I'll try and get to that soon.

I think this gets us back in the ball-park of where the patch was before all the ruckus.

I'll work on making the results page or something so browsers don't freeze up.

simpletest.test fixed.

So do we want to make test results page in some fashion? Or revert the new interface javascript expanding which appears to be the culprit.

Javascript doesn't expand in the posted patch, is that by design? Either way, this didn't crash by browser this time. Having the tests appear up top when they're finished is a nice thing. Not looked at the patch itself yet.

When it displays the results and has all the tests listed it seems to take a very very long time. Which is to be expected.

The results while running the tests don't expand more as that is too intensive for the browser and batch api.

I think it's good to leave the tests expanding until there's a way 'round the browser issues (if there ever is one). Just having the most recent test at the top saves a lot of scrolling up and down.

#142: expanding meaning on the end results page, not the batch API page?

@all: so where does this patch stand? thoughts?

This patch is a classic kitten-eater, with many iterations, each one slapping more stuff onto the existing functionality. So let's look at how to get this moving along.

For those who have not been following along, here is where the patch currently stands:

Batch API screen: http://img.skitch.com/20080925-n3cpgfgn6jxaw26qtbupncwa92.png

What this patch does:

1. Puts the newest stuff on TOP, rather than at the bottom.. no more scrolling!
2. Displays a summary of where the test run is, in terms of passes, fails, and exceptions.
3. Removes needless bold formatting of test results.

I would also like to see:

1. Each test result line should be prefaced with the group it belongs to. For example: Aggregator: Add feed functionality: 24 passes, 1 fail, 0 exceptions
2. The ability to click in to see more information about why a fail or exception occurred. I don't think we need this for passes. This might help with the freezing problems you were talking about on this screen.
3. The ability to stop a test run mid-way and return back to the admin/build/testing screen with the partial results.

Test result screen:
Before (top) Before (bottom)
After (top) After (bottom)

1. Checkboxes are supplied for filtering the list of results to show only errors and exceptions, for example.
2. Results are in a bulleted list, rather than a table, which makes them much easier to copy/paste into IRC and similar.
3. By default, only the result of the test is shown, but clicking into it will reveal more information such as the specific line and calling function.
4. Assertions made from within the .test file itself (as opposed to from a function in drupl_web_test_case.php such as drupalCreateNode()) are bolded.
5. See a summary of collapsed test results without clicking into them.
6. A BUTTON TO RE-RUN THE SAME TESTS!! YES!!!!!

Questions:
- Does it make sense to filter the list with passes hidden automatically?
- Is bolding test assertions an improvement? Is there another way we could bring these to peoples' attention?
- What is that "filter" select box for down below? Weren't the checkboxes for filtering?

I'll post how best to break this up in a second.

I see this logically being split up as follows:

Patch #1: Batch API screen improvements:
- Re-order test results at the top
- Remove needless strong tags on results
- Add little summary of test results
- Prefix test results with name of group

This could be committed in probably 5 minutes, assuming the code looks sound.

Patch #2: Results screen: Ability to re-run same tests
This is the #1 usability improvement we can make to SimpleTest module; the rest don't even come close. Let's break this off so we can get it committed quickly, as it will improve testers' efficiency by some astronomical percent.

Patch #3: Results screen: Compact view + Filtering
- Make test results show in bulleted list rather than a table
- Add the checkboxes at the top to filter the list by passes/fails/exceptions.
- Add "drill-down" effect so that you can see only the basics, with more details a click away.
- Add this same "drill-down" effect to the Batch API screen on test failures/exceptions to show information about the fail while the test is still running.

I'd advise turning this issue into Patch #3, unless you feel like closing it.

Patch #4: Cancel test run
A button or something on the Batch API screen that allows you to cancel a test run, and be returned to the results page with the partial test results.

I think this should give us a path where we can see some movement on this patch.

Patch still applies cleanly (with offset)!!!!!!!

I have started work.

#1: #314401: SimpleTest: Batch API Screen Improvements !

Here is #2 & #3 without folding on Batch API page. I'll have to look back in old patch for that.

This patch will contain bulk of clean-up code and the changes to make the fold out pertain to those functions anyway. So this will be the house-cleaning/ui patch.

The housecleaning is very much a part of UI changes so I would rather not try and separate them (at this point).

I'll look at breaking off the rerun form, but it uses things from the clean-up...(very intertwined).

Once these two-three patches are dealt with I'll open the cancel on batch API one.

Thanks for the thorough review.

@all: The folding js needs to be thought about as it kills browsers when running all/large numbers of the tests.

On batch API page only displaying errors is a good idea which I will work on, but for main result set we should take some time to figure it out.

30.18 KB + 7.49 KB = 37.67 << 47.4 39.91 KB (original). I know there was some stuff I removed, but that may come back when I add batch API folder.

i'm not a fan of the current user experience. i tried running a set of tests fliped to another tab to do some work and came back to find:

The tests have finished running.

sure it was in green so i'm guessing it worked okay. but that's a pretty useless message. at least tell me that they finished and all x of y passed. the nice thing about having them listed was you could look back and see what you ran. "did i run user or upload... crap i can't remember.... guess i've got to run them again to be sure"

??

Did you clear all your caches and such. You should see the results after running just like before, just in a different format.

I like the change! It's a pain to have to remember which tests failed when you go back to run the tests again though.... Also, I ran all tests (passed, by the way), and then clicked on some filtering options, and it stalled my browser. Not really a fan of showing 100% uncollapsed passing tests either. Would be nice to uncollapse just the failing tests.

Attempt at re-roll.

JavaScript doesn't work and I'm not into figuring it out. At this point this patch either needs to be committed or wasted. Splitting it up will causes lots of problems and create code that has never existed before and has no need of existing.

During the split up to clean the code the interface was also changed. So making a patch with only the split up is a bit much and visa versa.

did this get committed elsewhere?

Closing in favor of new issue: #445950: SimpleTest interface separation and cleanup