PHP notice in clickLink if link does not exist

In a case like this, I think a ternary is actually MORE readable.

Wouldn't it be better to have something like 'not found' as url_target so the message doesn't look like "Clicked link %label () ..."?

patch #3 it is then

Can we add a test for this to simpletest.test?

So this is my first attempt at writing a test!

After writing the test though, I was curious:

@@ -362,6 +362,16 @@ class SimpleTestBrowserTestCase extends DrupalWebTestCase {
+    $this->assertEqual($this->url, $base_url . '/', 'Clicked a non-existant link');

It felt strange writing this assertion. The behavior implemented in patch #3 is such that if clickLink() is passed a label that doesn't exist, it winds up returning a drupalGet() on an empty string, which is essentially $base_url. I'm not sure if this is the desired behavior, but my concern lies in tests that might assume clickLink() either returns drupalGet() on a link if it exists, or false if it doesn't.

I ran the test before and after applying patch #3 by using git checkout on drupal_web_test_case.php. The test (by itself) fails before patch #3, and passes afterwards.

Review is greatly appreciated :)

@jhodgdon, Thanks for the review.
Your assumption of how DrupalWebTestCase::clickLink works appears to me to be incorrect. I've used debug() to verify all values within the function when it is called with a bad link.

If DrupalWebTestCase::clickLink is passed a bad link, I've found the following sequence occurs:

1.     $urls = $this->xpath('//a[normalize-space(text())=:label]', array(':label' => $label));
   

   DrupalWebTestCase::xpath() returns an empty array if the xpath query comes back with no results. Therefore $urls is assigned an empty array.

2.     $url_target = isset($urls[$index]['href']) ? $this->getAbsoluteUrl($urls[$index]['href']) : '';
   

   The ternary operation assigns an empty string, '', to $url_target

3.     $this->assertTrue(isset($urls[$index]), t('Clicked link %label (@url_target) from @url_before', array('%label' => $label, '@url_target' => $url_target, '@url_before' => $url_before)), t('Browser'));
   

   This assertion will always fail due to the fact that isset($urls[$index]) evaluates to FALSE because $urls is an empty array.

4.     if (isset($url_target)) {
         return $this->drupalGet($url_target);
       }
   

   This condition will always evalutate to TRUE because according to php.net, isset($var):
   Returns TRUE if var exists and has value other than NULL, FALSE otherwise.
   
   An empty string is not NULL. Therefore DrupalWebTestCase::clickLink returns $this->drupalGet(''), which indeed is the HTML of the base URL. Using if (!empty($url_target)) solves this last problem.

There is a larger logic error which lies in the fact that DrupalWebTestCase::clickLink asserts that the link passed to it exists. One can test that DrupalWebTestCase::clickLink returns FALSE when given a bad link, however, in doing so the assetion within DrupalWebTestCase::clickLink itself will fail.

I'm assuming that one of the assertions must be removed. Attached is a new patch which addresses the issue of DrupalWebTestCase::clickLink not returning FALSE when passed a bad link. However, the aforementioned logic error still exists, and I'm not sure how to best handle it.

This is a straight reroll (of #13) for the new directory/file structure - it doesn't work.

And the actual reroll of the patch in #13... >_>

Alright, so Cottser said this is likely no longer a "Novice" issue (I also removed "Quick fix" as it's turning out to be not so quick a fix anyway...). I'm not confident at all with the SimpleTest system to make any calls on this, so I'll just leave it here.

However, kid_icarus is right with regards to his point about the logic. If you run a test on clickLink'ing a bad link, you will get an error from clickLink(). However, removing that assertion in clickLink will also not reveal bad links in places where a bad link is not supposed to be.

Currently, I'm thinking to either have clickLink tage a paramenter, e.g., $badlink, which could swap assertTrue with assertFalse if = TRUE (defaulting to = FALSE, to not break current tests (and also because that's the most likely scenario...)), or have the assertTrue check on something else; like $url_target or the $urls array - or perhaps just populate $urls with $index if it's empty? But then again, we do not want to open up for actually broken links... This is making my head hurt. :p

The two attached files are the same as the reroll from just before, only split up in the "fix" and its test. (No, they still don't work. :))

With this patch we could know if the link was correctly clicked.

@jhodgdon excuse about the if, I was trying different assertions and at the end I didn't verify very fine what I did. This is the same code but better.

I don't know if this test is the better option.Please let me know the best way to do this under your point of view.

upsss also this text is not correct in the patch 'Active URL persists after clicking non-existant link'

Like Freso says in #19 'This is making my head hurt. :p' trying to understand the best way to test when clickLink fail correctly... :))

Here we test if the link is correctly clicked (in this case we want that) and verify that the user login form isn't correctly validated so we are in the same url.

Also added the patch created in the previous comments.

Added a test to verify that a link that doesn't exist, is correctly no clicked.

Changed at assertTrue the t() with format_string.

At localhost the test pass correctly....

@jhodgdon about the test snippet ... What I want to do is to click correctly a link and check if the actual url is correct.

I really don't know the best possibility and also if we need it.

What do you think about it?

Ok so i'll create another patch without clickLink() test ... let me time to check the fails and exceptions correctly I need to debug what's going on here :) It seems that clickLink is adding another fail more ...

@jhodgdon:

We have more fails actually because when we try for example a drupalGet() from stubTest appear these fails:

Undefined index: backtrace
Notice errors.inc 188 _drupal_log_error() Exception
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd8.simpletest211209semaphore' doesn't exist: SELECT expire, value FROM {semaphore} WHERE name = :name; Array ( [:name] => variable_init )

Debugging with debug() I can see why it appears and this is why:

'Tables_in_d8' => 'simpletest907255simpletest211209semaphore',

In the setup we add the first prefix and then another new (second prefix) + table name.

So the tables are created but when trying to use them it's not possible.

I fixed the problem in the this patch.The problem is .... I really don't know when exactly we could need to use the first prefix and then the second... like actually is in the code.

Actually seems with no side effects with this patch.

Also we have two debug messages more added when we go to 'node' and another when clicking a link.

Perfect. I will add the space in the comment.

About the first point :

-        'default' => $value['prefix']['default'] . $this->databasePrefix,
+        'default' => $this->databasePrefix,

Actually with $value['prefix']['default'] . $this->databasePrefix we have 2 fails at stubTest when trying to click a link or ... log a user or something. Exactly this error:

Undefined index: backtrace
Notice errors.inc 188 _drupal_log_error() Exception
SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd8.simpletest211209semaphore' doesn't exist: SELECT expire, value FROM {semaphore} WHERE name = :name; Array ( [:name] => variable_init )

Ok, so I can verify debugging that the semaphore table name is 'simpletest907255simpletest211209semaphore', while testing we ask for 'simpletest211209semaphore'. The method name is changeDatabasePrefix(). I see that it ' Changes the database connection to the prefixed one. '.

What I don't understand is why we need to have a table with this name 'simpletest907255simpletest211209semaphore' and when exactly we need to have it.

If I don't remove the first prefix '$value['prefix']['default'] . $this->databasePrefix' then I will always have the 2 unexpected fails. Also I am trying to do the same but without modifying TestBase ...

1) So I would like understand why and when we need two prefixes because at the moment I don't understand why...
2) Do you think that there's another way to do this ???
3) Maybe this is a bug because we are asking for a table that doesn't exist.
4) Modifying TestBase the test passes correctly.

Thanks for your help.

Yes , I'll try in #drupal-contribute .

1) So I would like understand why and when we need two prefixes because at the moment I don't understand why...

SimpleTest tests itself using a browser test which means that it runs a test using the web runner and does so from a test. We must go deeper...etc etc, but yes it does.

2) Do you think that there's another way to do this ???

The assertions for the inner test require a database even if the test itself does not, something clever perhaps, but I doubt that will be any cleaner then the way it works now. Although I do not see why we need to explicitly prepend both prefixes as it should just be recursive.

I have not read the issue in length or looked at patches, just responded to #36 as linked to from IRC. Let me know if this is helpful or need more details.

Just hit this myself while porting Pants module from D7 to D8. Marked #1089792: Undefined variable: url_target in clickLink as a duplicate of this bug.

Just unassigning to free up (and bump) this issue. @marthinal, if you still want to work on this feel free to reassign. Thanks!

I am amazed by this discussion.

Can I convince people here that they have gone off track into a cul-de-sac?

I take a much simpler approach... if WebTestBase.php clickLink() is fed an invalid label it should abort with an exception to tell the test developer he/she is doing something wrong. Exceptions should be limited to when the routine cannot sensibly continue this is such a case.

That is what burn myself today and webchick a month ago and i guess jhodgdon ( OVER TWO YEARS AGO ) and nothing has been done!!!!!

Fundamentally this should be a ONE line fix...

I say again If clickLink() is given invalid input it should bark at the webtest creator appropriately.

  protected function clickLink($label, $index = 0) {
    $url_before = $this->getUrl();
    $urls = $this->xpath('//a[normalize-space()=:label]', array(':label' => $label));

    if (isset($urls[$index])) {
      $url_target = $this->getAbsoluteUrl($urls[$index]['href']);
+    } else {
+      throw new \UnexpectedValueException('Clicked link is invalid.');
+   }

    $this->assertTrue(isset($urls[$index]), String::format('Clicked link %label (@url_target) from @url_before', array('%label' => $label, '@url_target' => $url_target, '@url_before' => $url_before)), 'Browser');

    if (isset($url_target)) {
      return $this->drupalGet($url_target);
    }
    return FALSE;
  }

Behind the scenes it is more than an one line fix if you include the @throw comment and a web test that feeds invalid input into clickLink() and check for the exception.

I will work on this further if I can get consensus... but given the history this is unlikely

sorry for the rant.

https://drupal.org/comment/8538789#comment-8538789

I say again If clickLink() is given invalid input it should bark at the webtest creator appropriately.

Well, no. clickLink() should report a failed test if the link label doesn't exist. It should also report a fail if the URL under that link is unreachable.

Patch this evening.

Basically, this is the only change to clickLink():

    $this->assertTrue(isset($urls[$index]), String::format(
      'Clicked link %label (@url_target) from @url_before',
      array(
        '%label' => $label,
        '@url_target' => isset($url_target) ? $url_target : '<none>',
        '@url_before' => $url_before,
      )
    ), 'Browser');

This just says 'none' for the path if xpath can't find anything.

Also added a unit test so we can check the behavior when the link exists and when it doesn't.

We can't unit test (or simpletest) the output of the assertion, because assertions are not injectable, and you can't assert that a failing assertion is a pass. :-)

I agree with @martin107 in #43:

Throwing an exception makes sense, because if a link cannot be clicked, then the entire rest of the test method will not succeed either way.

In almost all cases, failing to click a link is an unmet precondition, so proceeding with the test method only makes you wait unnecessarily long(er) for test results, whereas the results will only consist of failures after the failing clickLink().

I consider those needless wait times to be even more annoying than the faulty fail/error handling of clickLink() itself. — In 99% of all cases I experienced myself, clickLink() failures are "D'oh!" situations. Instead of waiting minutes for the test to complete with tons of failures, I would have highly appreciated an early exception that halts the test (method), so that the broken clickLink() can be fixed first (and fast).

Throwing an exception makes sense, because if a link cannot be clicked, then the entire rest of the test method will not succeed either way.

Yes exactly. So FAIL and move on, but there is exactly zero need for an exception. Subsequent tests will fail also, but that's good, because this is, after all, a TEST. :-)

Here, let's get remedial:

Because clickLink() has an assertion in it, it is a test. It should really be called assertClickLink().

We're not trying to follow the link, we are trying to FIND OUT if we CAN follow that link, and then we do it if we can.

If the label doesn't exist, then clearly the test fails.

And in that circumstance, we're completely done. There is nothing else to say.

We are not in the midst of an error condition. We simply failed to find the link, click it, or follow it.

Again: Failing to find the label is not an error condition. Failing to follow the link is not an error condition. They are both FAIL conditions, which is different.

SimpleTest should hide errors related to discovering whether the test has passed or failed.

Because that's how testing works.

Also: My interest in this issue comes about from writing tests which are supposed to fail at this point, because that's what programmers do these days. You write tests which fail, and then make them pass.

Here's the failing test for a skeleton module I wrote: https://qa.drupal.org/pifr/test/764843

Note that it throws a bunch of errors which deliver zero value to the test author (me), or anyone else. These errors are fixed by the patch in #45, but the test still fails, because that's how testing works. :-)

Here's what you see if you get a failing test in Drupal due to clickLink().

It's completely unacceptable.

I want to suggest that there be a checkbox in the UI that says 'Report stack trace for failures,' which is turned off by default, but I doubt that if I do the work, anyone will want it.

If you want this, please ping on this issue.

@jhodgdon: There's a patch in #45 that could use a review, which fixes this error. It unit-tests clickLink().

Formatting according to coding standards. :-)

Basically it checks for $url_target and if it's not set, then subtitutes 'none'. This way we get the message even if the target isn't found, and we also get no crash.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -2391,7 +2391,14 @@ protected function clickLink($label, $index = 0) {
+        '@url_target' => isset($url_target) ? $url_target : '<none>',

We have a number of things going on in this issue.

We have a problem where SimpleTest has an actual bug. (It tries to use an uninitialized variable.)

We have a problem where people have confused this bug for useful debugging information. (Other fails present a stacktrace and error codes, so it seems like this one should, too.)

We have two solutions to the problem: 1) Martin107 wants to throw an exception so that we can get the debugging information, and 2) others and myself want to just see whether the link exists and then just report a fail.

My solution is very simple, fails on fail rather than throwing an exception, and has a unit test for clickLink().

If we can get an RTBC, then a maintainer can decide, and we can move forward with something other than that screenshot in #51.

This is an almost-blocker for #2277599: [meta] Port tests from D7 to D8 Examples, Add Skeleton Module Structure because of the extreme tedium it creates.

Your suggestions are good. :-) I'll re-do the formatting and pad out the test docs.

Needed a re-roll anyway, so here it is.

Reverted the formatting, which is still just as unreadable in the patch. :-) Also added more docblock and inline commenting to the test, and added another piece of test data.

What this patch does: It introduces a check to make sure that $url_target exists, and if it does not, it substitutes '<none>' for the path in the report line. It also adds a heavily-mocked unit test with data provider, which verifies that if $url_target doesn't exist, clickLink() returns a fail. Were clickLink to throw the error for this issue, PHPUnit would fail this test.

Here's a reformatted version of the original function for readability's sake, and then the changed version.

Before:

protected function clickLink($label, $index = 0) {
    $url_before = $this->getUrl();
    $urls = $this->xpath('//a[normalize-space()=:label]', array(':label' => $label));

    if (isset($urls[$index])) {
      $url_target = $this->getAbsoluteUrl($urls[$index]['href']);
    }

    $this->assertTrue(isset($urls[$index]), String::format(
      'Clicked link %label (@url_target) from @url_before',
      array(
        '%label' => $label,
        '@url_target' => $url_target,
        '@url_before' => $url_before,
      )),
      'Browser'
    );

    if (isset($url_target)) {
      return $this->drupalGet($url_target);
    }
    return FALSE;
  }

After:

  protected function clickLink($label, $index = 0) {
    $url_before = $this->getUrl();
    $urls = $this->xpath('//a[normalize-space()=:label]', array(':label' => $label));

    if (isset($urls[$index])) {
      $url_target = $this->getAbsoluteUrl($urls[$index]['href']);
    }

    $this->assertTrue(isset($urls[$index]), String::format(
      'Clicked link %label (@url_target) from @url_before',
      array(
        '%label' => $label,
        // This is the changed line...
        '@url_target' => isset($url_target) ? $url_target : '<none>',
        '@url_before' => $url_before,
      )),
      'Browser'
    );

    if (isset($url_target)) {
      return $this->drupalGet($url_target);
    }
    return FALSE;
  }

And here we see the test failing against the current code:

$ ./vendor/bin/phpunit --group simpletest
[..]
There was 1 error:

1) Drupal\simpletest\Tests\WebTestBaseTest::testClickLink with data set #0 (false, 'does_not_exist', 0, array())
Undefined variable: url_target

/Users/paulmitchum/projects/drupal8/core/modules/simpletest/src/WebTestBase.php:2526
/Users/paulmitchum/projects/drupal8/core/modules/simpletest/tests/src/WebTestBaseTest.php:189

FAILURES!
Tests: 18, Assertions: 27, Errors: 1.

You have to know what 'extant' means. :-)

Nevertheless... This is #58 with all the changes above, separate test patch to illustrate fail.

Check comment #58 to see a readable version of the changes.

I'd set this back to 'needs review' but then it would just test again and fail and be set at 'needs work.'

Ok, here's a re-upload of the patch, without the test-only patch.

Note that the failing test-only patch appears in #60 and the run-down of what this patch does appears in #58.

And here's what it looks like in the UI:

Compare with: https://drupal.org/comment/8843605

I agree with @sun that a test method should be halted the moment you get an error - how every other test method in the same class should run - however that is not how simpletest currently works so the change here is in line. Changing this behaviour is a separate issue (and one that I would support).

The major issue I have with this patch is the complexity of understanding the test and the fact that the test is not really covering much of the clickLink functionality. Yes PHPUnit is great and fast and everything but when we're mocking 5 methods and using reflection to change whether or not something is protected alarm bells should be ringing. This test will be expensive to maintain because it is tightly coupled to the internals of the clickLink() method.

+++ b/core/modules/simpletest/tests/src/WebTestBaseTest.php
@@ -86,4 +86,108 @@ public function testAssertFieldByName($filename, $name, $value, $expected) {
+        array(),
...
+        array(0 => 'this_is_a_url'),
...
+        array(0 => 'this_is_a_url', 1 => 'this_is_another_url'),
...
+    $xpath_array = array();
+    foreach ($xpath_data as $key => $xpath) {
+      $xpath_array[$key]['href'] = $xpath;
+    }

That said if we are going to go down the PHPUnit route, why are we not just passing in a correctly structure return value for xpath? Or even better not actually mock xpath but mock drupalGetContent() since providing content would make the test much easier to understand.

To improve the coverage of what is being tested we should be asserting if drupalGet() is called and what with.

@alexpott #68:

I agree with @sun that a test method should be halted the moment you get an error

There's no error. It's a test. It fails. Not an error. The desired behavior is to report a fail, not crash in your face. The current behavior is a bug.

The major issue I have with this patch is the complexity of understanding the test and the fact that the test is not really covering much of the clickLink functionality.

It exercises *every line* of clickLink() towards a number of desired results. Don't believe me? Here's a picture:

Note that a report like this is impossible using SimpleTest.

Yes PHPUnit is great and fast and everything but when we're mocking 5 methods and using reflection to change whether or not something is protected alarm bells should be ringing.

I'm using PHPunit because there is a veritable army of patches in this issue which failed to use SimpleTest to test this functionality. The unit test proves that clickLink() now does not do the thing it shouldn't do. Which is error out.

That said if we are going to go down the PHPUnit route, why are we not just passing in a correctly structure return value for xpath?

Either way works. Will change it.

Or even better not actually mock xpath but mock drupalGetContent() since providing content would make the test much easier to understand.

drupalGetContent() is not a dependency of clickLink(), so it doesn't need to be mocked.

To improve the coverage of what is being tested we should be asserting if drupalGet() is called and what with.

That's happening in the patch in #65 already.

Edit: Actually, yah, it's not, strictly speaking. :-) However, we are asserting that drupalGet() is called and returns a value. We don't care what it returns. We know what value is being sent to it, because we mocked getAbsoluteUrl().

Attached patch puts the 'href' in the xpath data, rather than adding it in the test.

@mile23 phpunit stops running a test method the moment an assertion fails :) It does not run any more assertions in the same method - I think simpletest (or any testing framework) should work the same way since after one assertion fails (especially one in click link) all bets are off as to what will happen next.

Exercising lines does not mean that all the logic in the method is tested - for example:

+++ b/core/modules/simpletest/tests/src/WebTestBaseTest.php
@@ -86,4 +86,106 @@ public function testAssertFieldByName($filename, $name, $value, $expected) {
+    $web_test->expects($this->any())
+      ->method('drupalGet')
+      ->will($this->returnValue('A Web Page.'));

We should assert this is called once and with the correct url and if there is no target url we should assert that it is not called.

And people have got code coverage reports out of simpletest https://www.drupal.org/project/code_coverage

@alexpott #71

@mile23 phpunit stops running a test method the moment an assertion fails :) It does not run any more assertions in the same method - I think simpletest (or any testing framework) should work the same way since after one assertion fails (especially one in click link) all bets are off as to what will happen next.

In an ideal world, yes, we'd short-circuit the whole method as failed on the first assertion fail. That's outside the scope of this issue though. I can't find an issue for such a change, but fire it up and let's go.

In this issue, we're going from buggy crash to consistent output.

RE: drupalGet(): We should assert this is called once and with the correct url and if there is no target url we should assert that it is not called.

That's exactly what we're testing with the unit test. In fact, it's the *only* thing we're testing with the unit test, because it's the only thing we can test. :-)

Our mocked drupalGet() can never return FALSE. We mock it because we're not testing the behavior of drupalGet(), and if we return a known value we know that it was called.

With all the mock scaffolding in place, clickLink() can only ever return 'A Web Page.' or FALSE. If we get 'A Web Page.' then we know drupalGet() was called. Thus, mission accomplished.

But... Here's a patch to make it more clear, substituting 'This Text Returned By drupalGet()' for 'A Web Page.'

@jhodgson:

Agreed that halting on fail is out of scope for this issue. Everyone can argue about that in some other issue. :-)

The code snippet you quote above is kind of garbled... Here's what I see in the patch.

+++ b/core/modules/simpletest/tests/src/WebTestBaseTest.php
@@ -86,4 +86,106 @@ public function testAssertFieldByName($filename, $name, $value, $expected) {
+  /**
+   * Data provider for testClickLink().
+   *
+   * @return array
+   *   Array of arrays of test data. Test data is structured as follows:
+   *   - Expected return value of clickLink().
+   *   - Parameter $label to clickLink().
+   *   - Parameter $index to clickLink().
+   *   - Test data which that be returned by mocked xpath(). Return an empty
+   *     array here to test the condition of no link found.
+   */

+++ b/core/modules/simpletest/tests/src/WebTestBaseTest.php
@@ -86,4 +86,106 @@ public function testAssertFieldByName($filename, $name, $value, $expected) {
+  /**
+   * Test WebTestBase::clickLink().
+   *
+   * @dataProvider providerTestClickLink
+   *
+   * @param mixed $expected
+   *   Expected return value of clickLink().
+   * @param string $label
+   *   Parameter $label to clickLink().
+   * @param int $index
+   *   Parameter $index to clickLink().
+   * @param array $xpath_data
+   *   Test data that will be returned by mocked xpath().
+   */

We're mocking xpath() so it returns known results for trying to find $label on the page. If we tell xpath() to return an empty array, then we're testing for the situation where no link was found.

Doing this, we can force clickLink() down different code paths based on the result of xpath(). The one we care about most is if there was no link found, because we want to be sure that our code catches any errors related to it.

The modification to clickLink() only catches an error condition. The test only proves that the error condition doesn't happen any more.

If someone feels a need for more testing of clickLink() beyond the scope of this issue, then open another one please.

Ahh, structure error sentence in the latter. Will fix that. :-)

As for the former:

So the first item in the array appears to be text returned by drupalGet(), but it is documented to be "Expected return value of clickLInk". That is not right?

drupalGet() is mocked to return a known value. It's 'This text from drupalGet()' because alexpott seemed to be unclear on whether we were checking if drupalGet() was returning a value.

If you do a little analysis of clickLink(), you see that it returns either FALSE or the value of drupalGet().

Therefore, we know that if clickLink() returns 'This text returned from drupalGet()', from our mocked version of drupalGet(), then clickLink() found the link.

So it's both the return value of the mocked drupalGet() *and* the expected output of clickLink(). :-)

Added some more explanation.

I hope the seemingly unrelated test fail from the last patch goes away... I couldn't get Drupal to install to make a test work and see.

@jhodgdon whilst not wanting to derail this issue any further - my point is that once there is a failure in one test method that method should no longer continue to run - and this is especially important for clickLink since if the assertion fails the simpletest browser is not even on the expected page anymore. The other methods contained in that class should still run. But if you've read my comments you'll see I'm not blocking this patch and agree that it should be fixed. All I'm trying to do is point out that the test we're adding is not actually covering all the logic in the clickLink method and the test is extremely expensive to maintain. Personally I would have preferred if this test had gone down the same path as the testAssertFieldByName() contained in the same test class - because understanding html is far far simpler than understanding xpath output and the actual input to the method is output from the simpletest browser - but y'all seem hellbent on this approach so whatever.

So here is what I'm on about - I've changed the test to now actually test all the logic contained within the clickLink() method. If we're testing that method it is important to assert that we don't ever try to use drupalGet() on a non existent link :) As we're adding a completely new test for clickLink() and using code coverage reports to say hey look we've covered everything it is important that we actually do cover everything.

Now for the patch

Personally I would have preferred if this test had gone down the same path as the testAssertFieldByName() contained in the same test class - because understanding html is far far simpler than understanding xpath output and the actual input to the method is output from the simpletest browser - but y'all seem hellbent on this approach so whatever.

If someone doesn't understand what xpath() is doing, why are they writing unit tests for this class? :-)

Your version is much better. I see what you were trying to goad me into doing.

In the past I've gotten into trouble for writing tests beyond the scope of the thing I changed. If the issue was 'fix the php error and expand test coverage,' then yah.

Anyway: I give it the Mile23 Seal Of Approval. Let's see if jhodgdon has further criticism.

Committed this patch to 8.x. I think the message could actually be made more clear when $target_url is not set but this is a lot better than what we have.

Incorporated @jhodgdon's change, which is way better. :-)

Small tweak to method docblock.

Added pass()/fail() requirement to unit test, which is kind of the whole point, right?

If #81 was really added but not pushed or something, it might need reroll.

Yah, I think an extra returnValue(NULL) crept its way in there in a copypasta frenzy.

As far as 'assert'... If you look at pass() and fail(), they just map to asssert()... Also, I think clickLink() should really be called assertClickLink(), but that's another issue for another day.

So: Removed extra returnValue(NULL), changed docblock wording for clickLink(), removed some docblock stuff from the test method that no longer applies.

Committed/pushed to 8.x, thanks!

Moving to 7.x for backport.

Patch for Drupal-7

Removed the extra space.

some testbot glitch

Committed to 7.x - thanks!

Pretty sure this could be tested for Drupal 7 (the test would just look different than Drupal 8) but testing bugs in the test framework internals doesn't seem like it's super high-priority, so someone can do that as a followup if they want to.