rfay asked me to file this here.

I am working on a patch here:

#1417320-4: Introduce the Request object

Both old and new tests run locally when I run them myself, both from the browser and from the command line. Testbot, however, is rejecting it, with the above message.

Randy indicated that it was the alternate simpletest module that testbot uses, or something like that. I don't fully understand the details.

The patch in question is messing with the path handling logic. Specifically, it is adding a new class, removing some functionality from arg(), and then eliminating some, but not all, uses of arg(). I thought I'd gotten all of the relevant ones, but it's possible there's something in the testbot that's still being cranky.

Let me know if you need more information. I'm just confused at this point.

Comments

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

I tried to debug this but ran out of time. When the internal browser tries to enable simpletest, $b->result['errors'] has 8 errors. I have a stash saved with some extra debugging on drupaltestbot654.osuosl.test.

jthorson’s picture

jthorson CreditAttribution: jthorson commented

1417320-request-object-review.patch:
PHP Fatal error: Class 'Drupal\\Core\\Request' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/bootstrap.inc on line 2731

1417320-request-object-test.patch:
After the $b->drupalLogin($user):

[16:19:48] Array
(
    [#pass] => 2
    [#fail] => 5
    [#exception] => 0
    [#debug] => 0
)

After the $b->drupalPost('admin/modules', ...):

[16:19:48] Array
(
    [#pass] => 4
    [#fail] => 8
    [#exception] => 0
    [#debug] => 0
)

I suspect your changes have borked the PIFRBrowser ... the 'simpletest' message is misleading, as it's just the first checkpoint after the review starts.

Will try adding some additional debugging within the PIFRBrowser itself.

jthorson’s picture

jthorson CreditAttribution: jthorson commented

Manually looking through the code, I could only find 6 assertion points instead of 7 up to the first results output from above ... but if I had to guess, I'd say you likely passed the drupalLogout() assertions (i.e. drupalGet works), but failed the drupalLogin assertion and the internal assertions during drupalPost().

Crell’s picture

Crell CreditAttribution: Crell commented

The -review patch is supposed to fail; the -test patch incorporates another patch from elsewhere that it depends on.

So... where do I look to fix that (comment 3)? Or is it something I can fix on my end?

jthorson’s picture

jthorson CreditAttribution: jthorson commented

Unfortunately, we've moved outside of my sphere of experience here (i.e. PIFR) ... and I now realize I was looking at the D8 version of DrupalWebTestCase, when the testbot is likely executing the D6 version.

I honestly don't know what the differences are between the two, but as a next step, I would probably look at the DrupalLogin() and DrupalPost() functions within the D6 DrupalWebTestCase version and see if we may need another D6 drupal_web_test_case.php patch in order to remain compatible with your changes ... or if it is even feasible.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@Crell, sad to say, this is a bit of a project. It appears that your patch has changed the login functionality, so the PIFRBrowser is unable to navigate a successful login.

Basically, what we have to do is figure out what assertions or actions in the login code are failing with this patch.

Crell’s picture

Crell CreditAttribution: Crell commented
Priority: Normal » Major

Well fork. This is a rather key step for, um, anything WSCCI. :-( Bumping priority. I'll try to poke through my code again soon, but I make no guarantees that I will be able to find the issue if there's a Drupal 6(!) version of something involved that I do not comprehend.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

OK. The problem will be a change in the login process, I think.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

Spent quite a lot of time on this tonight, and there are a series of things making it difficult.

The real problem is that the login is in fact failing, I believe. So that's $this->drupalLogin(). I suspect that something small changed about the form, but I haven't been able to detect it yet.

Actually... I think it's the drupalPost() itself that's failing. Has there been some change, for example, in form encoding?

Crell’s picture

Crell CreditAttribution: Crell commented

There shouldn't be, but anything that relies on arg(), current_path(), or request_path() may be affected. I will go through and try to port-forward more stuff and see if that fixes it. Of course, if the issue is with code running on d.o instead of in the checkout then there's nothing I can do about it... :-(

When I get a chance to work on this, expect me to post a ton of failing patches. :-)

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

At this point in the testing, the PIFRBrowser is not interacting with any *code* of the D8 patch. It's just doing things to make it work. Pulling the strings of the puppet. The problem is that for one reason or another, a post (apparently) doesn't work exactly the same way. Different headers in the response?

So it has nothing to do with the specifics of the D8 code. It has something to do with interacting with the patched D8 form submission, AFAICT. Probably some simple little thing.

Unfortunately, this piece of code is pretty isolated and really difficult to debug. I'm going to have to develop better debugging techniques to work with it. In regular simpletest you'd just have verbose and be able to see what was going on. But since this is the hacked out PIFRBrowser, you don't have that.

Crell’s picture

Crell CreditAttribution: Crell commented

So, drupal_web_test_case as far as I can tell is not using arg(), request_path(), current_path(), or ip_address() in the D8 OR D6 versions. That pretty much eliminates all of the things I'm touching, and any theories I have about how I could have broken it.

Unfortunately, this stalls WSCCI hard. I don't know what I can do with the request object if it's going to break testbot. Would anyone object to bumping this to critical?

chx’s picture

chx CreditAttribution: chx commented

give me ssh access to a testbot + procedure on how test this. i will unblock you. ssh key is at drupal.org/chx

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

@Crell, again this has nothing to do with code, as none of your code is actually executed. It has to do with the form submission or user login process.

@chx, I'm building 2 testbots right now, one for this and one for the security rollout.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented

I propose we do #1317528: Separate the testbot from the installer

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented
Status: Active » Fixed

chx++

jthorson’s picture

jthorson CreditAttribution: jthorson commented

And for posterity's sake ... the issue was that the patch broke Clean URLs, which is enabled on the testbots; but not used by simpletest when testing locally.

Status: Fixed » Closed (fixed)

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

chx’s picture

chx CreditAttribution: chx commented

For posterity, if you get this, try installing standard, then log out, log in and try to enable testing. We have seen the initial login fail and that is not reported back.

jthorson’s picture

jthorson CreditAttribution: jthorson commented

This 'login failure' issue should eventually get better when we deploy http://drupalcode.org/project/project_issue_file_review.git/commit/49924...