Provide branch testing status and retest options on Drupal.org project pages

I've created a new module, project_testing_status, which provides an 'Automated Testing' tab on the Project page. The contents of the tab consist of a drop-down box with the available releases for a project, as well as the testing results for the currently selected release.

This tab will appear for logged in users, if the project has any official releases.

The code is currently available in a sandbox at http://drupal.org/sandbox/jthorson/1238958. Once it's been reviewed, and assuming there is some buy-in for moving this to d.o, I'll promote the project and create an official release so that we can open an infrastructure ticket to request d.o deployment.

Pictures attached.

I've enabled the module on http://drupal.drupalorg.dev2.c--g.net; you can log in there with your regular d.o password (or contact me and I'll get you in). Note that clean urls are not enabled, so that can be confusing if you're a direct URL typer.

It seems to work wonderfully. I noted some discrepancies in #1277258: Terminology discrepancies and confusion.

+1

I took a tour through the code and found nothing objectionable.

Slick! In general, this would be a great feature. We'd love to make testing and test results more visible on d.o as a module health metric. As soon as this lands, we'll want a spin-off issue about stuffing some kind of summary of this data into the solr document for each project so that we can do ranking and filtering of contrib based on the presence of tests, if they're passing, etc.

In terms of the screenshots in #1, any reason not to just have a table with 1 row of status results for each supported branch? Not sure why we need a form on this page at all. Is the worry that the table will be info overload? Seems like there aren't going to be *that* many branches at any given time, and it'd be easier to just display the data instead of asking users to navigate form elements. Plus, might be nice to compare test results across branches, which the current UI makes very difficult since you can't see the results next to each other.

I'm also slightly concerned about "tab-bloat" on project pages. The bluecheese theme doesn't make those local tabs particularly visible in the first place (so folks might not even see this up there). And then there's the problem of how many tabs we've already got. I have nothing constructive to propose as an alternative, but I'm just sharing my concern in case anyone has a bright idea of a better way/place to display this data on project pages. ;)

In terms of the code: it's not obvious why this is a whole separate project/module. But, I'm far removed from the PIFR development universe, so my opinion doesn't really count for much. ;) Therefore, I'll just shut up about that and let y'all decide the best place for this code to live. I haven't looked at the code itself, so I can't enumerate any objections there.

Thanks,
-Derek

I went with the select box for a few reasons:

• to futureproof the ability to support testing of non-release branch/tags once we can support them via the testing infrastructure (as a complete branch+tag table could get quite large relative to the 'releases' list),
• to make effective use of screen real-estate, since the vast majority of releases/tags/branches currently have no test results to display ... and generally, only the latest branch/release results have any relevance,
• to provide UI consistency with the git instructions tab at the same location,
• and because I'm a lazy developer who used project_git_instructions.module as a base for the code, and that's how they did it!

As for the 'seperate module' approach, I was simply trying to avoid colliding with the current project_dependency efforts going on with PIFT, and thought doing this as a standalone might be less confusing than trying to add yet another moving piece to the puzzle. In addition to not muddying the waters for the rest of the PIFT work, this avoids creating an artificial dependency between this and the project_dependency work ... so that an issue or concern with one doesn't cause delays for the other. That said, folding this into the PIFT module directly does make a lot of sense.

Re: select box:

- git instructions has a select box since the entire contents of that page changes depending on what branch you said you want to work on (since all of the examples are updated to reflect the currently active branch). That makes sense for that page. In this case, it makes less sense. ;) It's just a table with some rows where each row has the same columns but potentially different values. Tables are good for that sort of info...

- Nothing says you have to display results for all branches and tags. The table would only have rows for branches that *have* tests.

- Nothing says you must restrict to release branches in the table. Granted, a project that goes nuts with tons of feature branches and other stuff, each of which has testing enabled, could get messy. To mitigate that mess, you could certainly order the table so all the mainline release branches are at the top. Point is, I'd rather not make the experience more clumsy for the vast majority of projects just in case some day there are projects with lots of branches with test results.

Stepping back: I'd love to see some thought go into if/how this integrates/interacts with the "Releases" subtab when editing a project, e.g.:

http://drupal.org/node/313122/edit/releases

and also the "Testing" fieldset on the "Issues" subtab (which is itself a bit of a confusing place for that to live, especially now that testing it's just tied to patches in the issue queue). It'd sure be nice to think about this stuff a bit more holistically, come up with a good plan for all these various testing-related UI elements and how they all fit together into a sane and grokable whole, and then have patches or modules that implement various aspects of the plan. There's an equivalent problem with managing releases themselves -- it'd be great if we didn't replicate that mistake with all the testing-related UI on project nodes.

@dww #1277756: Just show all branches on page (no select process)

@rfay: Cool, thanks. I replied there.

Cheers,
-Derek

I'm not sure it makes sense to tie this in with the 'releases' tab ... I've been working hard to remove the test/release dependency in the rest of the testing infrastructure so that it can be leveraged for sandboxes, core initiatives, forked repositories, etc ... all things which probably do not have associated releases.

That said, it may make sense to move the current 'Enable Testing' checkbox on the 'Issues' subtab over to a per-release 'enable testing' checkbox in the Releases subtab/table ... though changing the logic from 'per project' to 'per release' might also require some associated refactoring inside of PIFT.

Sorry I wasn't clear. I'm not talking necessarily about tying this directly to the releases table. However, from the perspective of a project maintainer, it'd be nice if there was a single "dashboard" where you manage your branches and releases and stuff. It's already a bit confusing that there's the page I linked and screenshot'ed above, but there's nothing there to add new releases, etc. From a code perspective, what's happening with this project is probably fine. But from a drupal.org UI/UX perspective, it's just making a bad situation (which is mostly my fault) worse (which is your "fault"). ;) Which is why I'm raising it here. That doesn't mean I want to torpedo this effort (far from it). But, I'm asking if we can at least think about things from the "I just maintain projects on d.o" perspective and consider what would be best for them, instead of only thinking of things from the "I maintain the Project* codebase" or "I maintain the PIFR codebase" perspective and consider what's best for us. Make sense?

Thanks,
-Derek

p.s. Another example of a setting related to Branches and Stuff(tm) that needs a good home is here: #1074960: Let maintainers set a default branch on git repositories. If we're not careful, that's going to live on Yet Another Tab or something. That's the situation I'm trying to resolve, ideally before we just deploy everything separately and then try to unify things at some later (perhaps never to arrive) date when everyone will be confused since things moved around again. If it was easy to Do It Right(tm) in the first place and make the UI not crazy, that'd be better, so that we don't roll out new features and then re-shuffle everything a few weeks/months down the line. See what I mean? That said, these are all not exactly related features, and they're all coming from separate modules (for good reason) so it's harder to get all the moving pieces to play nicely together.

I'm not demanding that everything be unified before we deploy anything.

I'm just requesting that we think about the d.o contributor's experience before we plow ahead with the Easiest To Implement(tm) solution and say "we'll worry about usability and user experience later".

Thanks,
-Derek

Gotchya ... thanks for the clarification.

Sounds like we need to ask someone on the Prairie Initiative to show us the end-game vision, so that we can make sure we're aligning to it in the long run. *wink*

Subscribing!!

Refactored code now available in sandbox repo, and demo available at http://rfay.redesign.devdrupal.org/node/3060/testing-status

I like it.

Went to a project that has testing for both 6 and 7 (role_change_notify) and note that it doesn't show or offer to test 7.x-1.x (because this is a sanitized, limited site?). Also, it only offers tags for testing, not branches. Should it offer those tags? Will people want to test tags?

Currently, it restricts the list to releases only ... there's code in the project to list all branch/tags instead of just releases, but until PIFT can officially support tags, I call the 'get releases' function instead of the 'get versions' function.

Hmmm ... that doesn't explain why 6.x-2.x didn't show up ... I'll have to dig into that again!

Um ... brainfart. It's already showing a test result for 6.x-2.x.

7.x-1.x-dev doesn't show up in the versioncontrol_labels table for that project, so it didn't get included it in the list. I'm assuming that all releases (-dev included) should be in this table (edit: And this case was simply due to having a truncated database); if this is incorrect, then please enlighten me. :)

K ... setting back to 'needs review'.

I think I'm leaning towards leaving this as a seperate project, rather than folding it into PIFT ... the main reason behind this is that I had to hardcode a drupal.org specific taxonomy id check into the code, in order to distinguish between module nodes and project nodes; and I'd prefer not to pollute PIFT with d.o specific code.

If I'm not mistaken we should move this over to infra queue; When it's RTBC it should be tagged "needs drupal.org deployment"

Code has been refactored to remove the drupal.org specific code ... now adds a 'select project types for which to display test results' option on the PIFT admin settings page.

Seeing that this is really just exposing the existing PIFT data on a site running PIFT, I think it makes more sense as a feature addition to PIFT (or pift sub-module) as opposed to a seperate project on it's own.

I've refactored the module into the following patch against PIFT. I know that we just recently packaged a new PIFT release, but it would be really nice to include this in the next iteration deployed on d.o.

Note: After applying the patch, you'll need to go to /admin/project/pift and select the project types for which the testing tab should appear (under the 'Results Display' fieldset).

Yay, I think that's a wise move. Thanks, jthorson!

Once this is committed and the feature request is marked "closed (fixed)", you can then add the "needs drupal.org deployment" tag to indicate that a fixed change that's already "upstream" in the Git repo is ready to actually deploy. If there's a new point release at that time, great, indicate what tag it's included in in the comment that adds the tag. If not, we can always deploy from the end of a branch.

Cheers,
-Derek

Slight tweak ... lack of results was throwing a PHP watchdog warning.

Tagging.

Patch still applies against 6.x-1.x-alpha6.

This version should also address the 'retest' link leading to /node, and empty $title variables in the drupal_set_message notification.

Re-roll ... missing a file.

And ... still missing in #29.

Redirect still broken.

Yet another attempt.

Sooo ... incredibly ... close ...

Finally, this one works.

May be resulting in duplicate entries in pift_test ... need to sleep on this one.

Duplicate entries were due to PIFT and PIFR auto-increment counters getting out of sync. Back to 'needs review'.

Also deployed on rfay.redesign in conjunction with scratchtestbot, ready for testing.

Occasionally, a branch tests seems to get stuck in a 'sent' state ... on other words, PIFR completes the testing, but PIFT does not update the status of the test. Because the 're-test' link was only enabled for tests with pass/fail status, there was nothing that a normal user could do to un-stuck this scenario.

This patch enables the 're-test' link for tests in the 'sent' status as well.

No good ... re-test routine explicitly fails if the test is in the 'sent' status.

New approach is to add a 'delete' link when in the 'sent' state, which is where the tests appear to get stuck.

... except that it keyed off the wrong status. *grrrrr*

... and the 'delete' button was marked 're-test'. *rolls eyes*

Siily question ... For performance reasons, I had been instructed it was better to use

$types = project_get_project_types();
foreach ($types as $key => $project_type) {

rather than 

foreach (project_get_project_types() as $key => $project_type) {

I am not sure how PHP does caching and which is more performant in the above example. Thoughts?

@Lars Toomre I would be very surprised if there is a relevant performance difference in those.

In so much of Drupal PHP code performance issues become irrelevant because of how much database work goes on. It's quite unusual that a difference like this (if it matters at all) is relevant to overall performance.

Code structure, algorithm choice, and limiting database accesses matter a lot. PHP oddities, not so much.

As I said it was a silly question... However, isn't the first alternative in #40 generally better?

Just took this for a test ride on rfay.redesign.devdrupal.org (test rides can be arranged for anybody, or do a drush upwd over there to set your password).

I was delighted by everything I saw. It all worked perfectly. Made me want to do all kinds of feature requests. I haven't studied the code yet, but fully support this. Looks great to me.

A long-needed related feature request that we may want to graft onto this UI later (so we should maybe think where it goes): #791818: Contrib modules need to be tested regularly against core (and notify maintainer).

sub

These are all nits, and IMO this is pretty well ready to go. I also think it's low-risk for d.o. But perhaps you'll be willing to discuss some of these.

+++ b/pift.module
@@ -84,18 +84,59 @@ function pift_menu() {
-    'title' => 'Request to re-test a file',
+    'title' => 'Request to re-test a file or branch',

How about "Request retest of a file or branch" ?

+++ b/pift.module
@@ -84,18 +84,59 @@ function pift_menu() {
+    'title' => 'Request to delete a test',
+    'page callback' => 'drupal_get_form',

Request deletion of a test ?

+++ b/pift.module
@@ -84,18 +84,59 @@ function pift_menu() {
+  $valid_type = array_intersect(variable_get('pift_results_display', array()), array_keys($node->taxonomy));
+  if ($node->type == 'project_project'
+    && !empty($valid_type)
+    && !empty($node->versioncontrol_project['repo']->vcs)
+    && $node->versioncontrol_project['repo']->vcs == 'git'
+    && user_is_logged_in()
+    && pift_get_releases($node, TRUE) != array()) {
+      return TRUE;
+  }

IMO we should have a permission associated with this as well. We may want to open this up to everybody, or not...

+++ b/pift.module
@@ -204,6 +245,64 @@ function pift_versioncontrol_git_refs_updated($repository, $refs) {
+      if (in_array(substr($data->name, 0, 1), array('6', '7', '8'))) {

Hate to do this. Let's make it more resilient somehow. It will be a short time before this is a bug.

+++ b/pift.pages.inc
@@ -94,19 +94,38 @@ function pift_pages_retest_confirm_form(&$form_state, $test_id) {
+          'title' => "Test " . check_plain($test['test_id']),

I haven't really checked, but is $test['test_id'] user-entered code? Do we need to check_plain it?

+++ b/pift.pages.inc
@@ -167,6 +188,50 @@ function pift_pages_retest_confirm_form_submit($form, &$form_state) {
+        'title' => "Test " . check_plain($test['test_id']),

Same question (needs check_plain()?)

+++ b/pift.results.inc
@@ -0,0 +1,304 @@
+ * Provide the Automated Testing tab and UI functions.
+ *
+ * @author Jeremy Thorson ("jthorson", http://drupal.org/user/148199)

Do we usually do this? I'm not in the habit... But I don't know.

+++ b/pift.results.inc
@@ -0,0 +1,304 @@
+ * @param $form_state
+ * @param $project object The project node object
+ * @param version string The currently selected version
+ * @param versions array List of versions
+ * @return array
+ */
+function pift_results_test_branch_form(&$form_state, $project, $versions = array()) {

Doc params and actual params don't match. ($version)

Oh... and how does this work with $form not being the first param? Is this not called by drupal_get_form()?

+++ b/pift.results.inc
@@ -0,0 +1,304 @@
+  $result = db_query("Select label_id, name from {versioncontrol_labels}
+          where repo_id = %d and name IN (" . db_placeholders($versions, 'text') . ")",
+          array_merge( (array) $repo_id, $versions));

It seems that Drupal practice is to capitalize SQL reserved words.

+++ b/pift.results.inc
@@ -0,0 +1,304 @@
+  $sql = "Select * from {pift_test} where type = 1 and id IN (" . db_placeholders($label_ids, 'int') . ")";

And in all of these.

+++ b/pift.results.inc
@@ -0,0 +1,304 @@
+          where project_nid = %d and label_id IN (" . db_placeholders($label_ids, 'int') . ")",

Just want to make sure here that $label_ids is not user-entered code or gameable, or this would be a security issue.

+++ b/pift.results.inc
@@ -0,0 +1,304 @@
+function pift_results_format_testresult_row($test = array()) {

A silly nit, but we're building the row, not formatting it right (as theme() is doing that for us)? So maybe pift_results_build_testresult_row()?

+++ b/pift.results.inc
@@ -0,0 +1,304 @@
+      if (user_access('pift re-test files') && $test['status'] > 2) {
+        $operations[] = l('Re-test', 'pift/retest/' . $test['test_id']);
+      }
+      if (user_access('pift re-test files') && $test['status'] == 2) {
+        $operations[] = l('Delete', 'pift/delete/' . $test['test_id']);
+      }
+    }
+    // Hide old results if new test queued/sent
+    if ($test['status'] < 3) {

I really hate our use of numeric literals in PIFT and PIFR. Can we use a define here (and everywhere else). I would rather these were strings in the database even, but we're not going to do that now.

Oh... and how does this work with $form not being the first param? Is this not called by drupal_get_form()?

As the form builder function, it's the responsibility of pift_results_test_branch_form() to return the $form. $form_state is an argument to form builders, but not the $form itself.

---

+++ b/pift.results.inc
@@ -0,0 +1,304 @@
+          where project_nid = %d and label_id IN (" . db_placeholders($label_ids, 'int') . ")",

Just want to make sure here that $label_ids is not user-entered code or gameable, or this would be a security issue.

Using db_placeholders() like that is exactly how to prevent SQL injection in case $label_ids are user-generated. Not using db_placeholders() and directly including $label_ids in the query string would be the security hole.

---

Otherwise, I agree with all of rfay's review points here.

Re: @author: Yeah, some people feel strongly that including @author is a bad thing since it encourages people to think someone else "owns" that code and that they shouldn't contribute to it. It's also weird once people start patching the file, how much ownership rfay should have after a thorough review like this, etc. See http://groups.drupal.org/node/81679 for more.

Cheers,
-Derek

How about "Request retest of a file or branch" ?
- Changed

Request deletion of a test ?
- Changed

IMO we should have a permission associated with this as well. We may want to open this up to everybody, or not...
- Added

Let's make it more resilient somehow. It will be a short time before this is a bug.
- Added pift_valid_prefix_list, which loads from the terms as selected in the pift_core variable. Still need to add new versions to the setting form each major release, but at least you won't need a change here.

I haven't really checked, but is $test['test_id'] user-entered code? Do we need to check_plain it?
- $test_id is user submitted ... which is why I added the check_plain. Of course, $test_id != $test['test_id'] ... so it's been removed.

Do we usually do this? I'm not in the habit... But I don't know.
- Seemed to be the standard for the other PIFT files ... I don't normally do it either. In this case, it was a cut-and-paste file doc block.

Doc params and actual params don't match. ($version)
- Fixed

It seems that Drupal practice is to capitalize SQL reserved words.
- Fixed

A silly nit, but we're building the row, not formatting it right (as theme() is doing that for us)? So maybe pift_results_build_testresult_row()?
- Changed

I really hate our use of numeric literals in PIFT and PIFR. Can we use a define here (and everywhere else). I would rather these were strings in the database even, but we're not going to do that now.
- Changed to use PIFT_STATUS_SENT

Uploading patch ... still need to validate it on qa.scratch.

Re-roll ... previous patch had two minor issues - with major consequences.

Committed to 6.x-2.x (0f1228a)

Just a ping on #791818: Contrib modules need to be tested regularly against core (and notify maintainer), which is so closely related. We can just add a checkbox, right "I want to be notified by email"