Full test being done with manual interface when only the branch in question should be tested [#1231918]

Comment	File	Size	Author
#23	pifr.add-directory-argument-on-manual-branch-tests-1231918-5.patch	2.09 KB	jthorson
#20	pifr.use-scan-instead-of-syntax_files-for-simpletest-patch-runs-1231918-20.patch	1.31 KB	jthorson
#19	pifr.use-scan-instead-of-syntax_files-for-simpletest-patch-runs-1231918-19.patch	1.25 KB	jthorson
#10	review.test_17.pifr_simpletest.role_change_notify.junk_.patch.1312122092.log_.txt	30.68 KB	rfay
#10	role_change_notify.junk_.patch.txt	322 bytes	rfay
#5	pifr.add-directory-argument-on-manual-branch-tests-1231918-5.patch	2.09 KB	jthorson
#4	pifr.add-directory-argument-on-manual-branch-tests-1231918-4.patch	2.09 KB	jthorson
#3	pifr.add-directory-argument-on-manual-branch-tests-1231918-3.patch	1.95 KB	jthorson

Comment #1

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 28 July 2011 at 03:33

I've also been getting tests against core with patches for a new contrib project that was started today

Log in or register to post comments

Comment #2

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 28 July 2011 at 03:40

@DaveReid, unless you're doing the manual testbot interface, for the testbots that's definitely #1126112: Full tests of Drupal being done for some contrib patches

You know we're pretty close to doing the dependency thing though (which I think fixes that one). Yay. We'll deploy one round of fixes shortly to QA and the testbots, and when that's settled, will push for deployment of project_dependency and the minor pift change it requires.

Log in or register to post comments

Comment #3

jthorson CreditAttribution: jthorson commented 29 July 2011 at 04:32

Status:

Active

» Needs review

File	Size
pifr.add-directory-argument-on-manual-branch-tests-1231918-3.patch	1.95 KB

Since the testbot doesn't know the project name, it isn't setting test.directory.review.

I thought about parsing it from the git repository provided, but that won't work for sandbox repositories ... so instead, I've added a new field to the manual review form. You'll now need to supply the module's folder name as well as the repository and branch.

Log in or register to post comments

Comment #4

jthorson CreditAttribution: jthorson commented 29 July 2011 at 05:05

File	Size
pifr.add-directory-argument-on-manual-branch-tests-1231918-4.patch	2.09 KB

Ooops! Argument needs to be an array.

Verified on local testbot ... just make sure to get the directory correct, or no tests will be run. Which reminds me ... need to add a check that --file argument actually contains valid tests ...

Log in or register to post comments

Comment #5

jthorson CreditAttribution: jthorson commented 29 July 2011 at 05:09

File	Size
pifr.add-directory-argument-on-manual-branch-tests-1231918-5.patch	2.09 KB

Another tweak ... new form field wasn't displaying the full 'description'.

Log in or register to post comments

Comment #6

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 31 July 2011 at 14:07

This one loses the "Interrupt any existing tests in progress" checkbox, which I found myself needing most of the time. (And it should probably be set by default).

I'd wish that this remembered the settings between invocations, as they're almost always repetitive. But global variable_gets don't seem like a very good idea.

Log in or register to post comments

Comment #7

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 31 July 2011 at 14:08

Status:

Needs review

» Reviewed & tested by the community

But it *works* YAY!

Log in or register to post comments

Comment #8

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 31 July 2011 at 14:14

It doesn't seem to know when it's completed. For example, running a branch test of role_change_notify, and monitoring admin/pifr, after completion, it always says "Status: Running tests, 66 assertion(s).", never knows that it actually got done. The invocation page *has* finally returned at this point.

Edit: And I have to explicitly "reset tests" to move run another test.

Log in or register to post comments

Comment #9

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 31 July 2011 at 14:16

Doing a patch test on role_change_notify, I get

    Notice: Undefined index: interrupt in pifr_client_review_form_validate() (line 171 of /home/rfay/workspace/testbot/sites/all/modules/project_issue_file_review/client/pifr_client.review.inc).
    Unable to test due to an existing test in progress (t: 147784).

Log in or register to post comments

Comment #10

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 31 July 2011 at 14:26

Status:

Reviewed & tested by the community

» Needs work

File	Size
role_change_notify.junk_.patch.txt	322 bytes
review.test_17.pifr_simpletest.role_change_notify.junk_.patch.1312122092.log_.txt	30.68 KB

And, as far as I can tell, this doesn't work on a patch test. Attached log and the junk patch I used.

Log in or register to post comments

Comment #11

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 31 July 2011 at 14:31

I'm also not sure about the value being offered by having a multistep form. Can you discuss that? Whenever there's a #fail it introduces complexity and new problems.

Log in or register to post comments

Comment #12

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 31 July 2011 at 15:18

BTW: If we can solve this one, can't we also solve #1126112: Full tests of Drupal being done for some contrib patches even without getting the dependency information?

Log in or register to post comments

Comment #13

jthorson CreditAttribution: jthorson commented 31 July 2011 at 20:52

The multi-step form was to prevent having to display simple-test specific options on coder tests, and vice-versa.

Log in or register to post comments

Comment #14

jthorson CreditAttribution: jthorson commented 31 July 2011 at 21:08

Regarding comment #10, this is due to syntax_ignore().

Your patch only affected patch.txt ... so that was the only file in the original $file array. The call to syntax_ignore filters through and removes any files from the $files array that don't match the passed 'test.extensions':

[test.extensions] => Array
  (
    [0] => php
    [1] => inc
    [2] => install
    [3] => module
    [4] => test
  )

Give it a shot with a patch on a php file.

On the other hand, what is the expected logic? Shouldn't the 'review' operation be doing a full run of '.test' files for the module, and only the 'syntax' operation be concerned with only affected files?

Log in or register to post comments

Comment #15

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 31 July 2011 at 22:40

That's really interesting. I would expect it to continue to to testing regardless of syntax.

Log in or register to post comments

Comment #16

jthorson CreditAttribution: jthorson commented 1 August 2011 at 02:23

#6 ... I can't reproduce. I always get the interrupt box at admin/pifr/run-tests

#8 ... Can't reproduce. My local testbot resets to idle.

#9 ... Can't reproduce ... though I'll open a ticket with patch to make sure it's defined before checking the value.

#10 ... Confirmed (Fail response with empty --file argument patch applied). Will chase this tonight.

#14 ... explains syntax check on 0 of 0 files, but not the review.

Log in or register to post comments

Comment #17

jthorson CreditAttribution: jthorson commented 1 August 2011 at 03:04

Okay ... for #10, here's what's happening:

pifr_simpletest.client.inc review() calls test_list().
test_list() matches the arguments['test.directory.review'][0] condition, and calls pifr_drupal.client.inc syntax_files().
pifr_drupal.client.inc syntax_files() doesn't match the arguments['drupal.modules'] or argument['directory'] conditions, so it calls client.inc syntax_files().
client.inc syntax_files() calls apply_files(), which contains the patch file which was uploaded.

So instead of a full directory scan, you get only the patch file.

Log in or register to post comments

Comment #18

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 1 August 2011 at 03:05

I'm pretty sure #6 was a result of me being confused by the multistep form.

Log in or register to post comments

Comment #19

jthorson CreditAttribution: jthorson commented 1 August 2011 at 04:19

File	Size
pifr.use-scan-instead-of-syntax_files-for-simpletest-patch-runs-1231918-19.patch	1.25 KB

Let's try this ... removed the syntax_files() dependency inside the test_list() routine, opting instead to scan directly. (Reversed some changes introduced in #746730: Allow test.directory.review to be an array.)

Log in or register to post comments

Comment #20

jthorson CreditAttribution: jthorson commented 1 August 2011 at 04:58

File	Size
pifr.use-scan-instead-of-syntax_files-for-simpletest-patch-runs-1231918-20.patch	1.31 KB

Previous patch had some path issues ... this one should work better.

Log in or register to post comments

Comment #21

jthorson CreditAttribution: jthorson commented 1 August 2011 at 06:09

Tested with manual branch and patch tests, and with automated branch re-test ... test from PIFT didn't work, as role_change_notify is being sent with 'master' as the branch to test (though 'master' is empty).

Log in or register to post comments

Comment #22

jthorson CreditAttribution: jthorson commented 1 August 2011 at 06:25

Status:

Needs work

» Needs review

Test from PIFT goes into 'postponed' state ... but confirmed (via manual retest) that a patch against token 6.x-1.x did not run the full suite of tests.

As the manual tests are fixed, I believe this item is ready to close. Will cross-reference to this patch in issue #1126112: Full tests of Drupal being done for some contrib patches, and continue the testing there.