Default message for linkNotExists and linkByHrefNotExists is wrong

I applied the patch and here are the screenshots.

Patch applied correctly in linkNotExists() and linkByHrefNotExists() functions

Can we move the status to "Reviewed & tested by the community"

Ooh, this gets tricky and confusing for sure.

1. +++ b/core/tests/Drupal/Tests/WebAssert.php
   @@ -247,7 +247,7 @@ public function linkExists($label, $index = 0, $message = '') {
   -    $message = ($message ? $message : strtr('Link with label %label found.', ['%label' => $label]));
   +    $message = ($message ? $message : strtr('Link with label %label found, but it should not.', ['%label' => $label]));
   

   So maybe "No link with label %label"? (and ditto for the other for href)

   (Out-of-scope side: strtr? Why on earth? Anyway.)

2. Since this is really easy to invert in one's head (as evidenced by the fact that the bug is here in the first place), we should actually provide a failing test case to prove the new message is the right way around. But then, on the other hand...
3. The failing test demo would also be good to do for the legacy trait that extends this and the positive assertions, to make sure that it isn't only half-inverted.

Hopefully that is not too much, but it would help prove the fix is correct.

Also IMO this isn't minor; it could actually screw up your debugging or test coverage because the message is essentially telling you what is the opposite of true. So promoting to normal.

Finally, can and should backport. While it is a "string" change it is no longer translated and we don't guarantee the translation of developer output.

Thanks and good find!

Also-too, we should compare to what the PHPUnit method that's being overridden said. Sorry that this is so many things to test for an essentially two-line patch. But this is an area where computers are much more reliable than our brains.

@GoZ, thanks!

There is something wrong with your snippet. Patch is about linkNotExists() and not linkExists(). For the text added 'but it should not', i'm base on other *NotExists methods in WebAssert class like

There is nothing wrong with the snippet exactly; that's just unfortunately how Dreditor pastes it since it tries to guess what the start of the function is from the diff header. The point is the change to the assertion message I've requested. Sorry if the default Dreditor formatting distracted from that.

Those methods don't exist in PHPUnit. It's Drupal specific.

This specific method is Drupal-specific. However, it (and everything else in BTB/WebAssert and AssertLegacyTrait) leverage PHPUnit and its assertion functionality. But actually what I said had a typo; sorry for the confusion. What I meant was to compare with the previous messages from before PHPUnit and use that pattern, e.g.: https://api.drupal.org/api/drupal/modules%21simpletest%21drupal_web_test... (the D7 version).

#10 was quite a bit more than I meant (and I meant a test patch to demonstrate the patch on the issue), but it also proves the point just as well. So that is good. Thanks! The one other bit from #7 was making sure the also matched. I just confirmed that myself.

So, just NW now to actually update the message test to just add the word "not" in the correct places like https://api.drupal.org/api/drupal/modules%21simpletest%21drupal_web_test..., rather than the current message which is not quite grammatical.

Also @boaloysius, I didn't mention this before, but now I saw it on another issue you contributed to as well. Posting screenshots of code in your IDE isn't helpful really. We don't need you to prove that the patch applies; testbot does that for us. We can always just read the code ourselves with the patch applied, also. Screenshots are primarily useful when testing user-facing patches. What is helpful though is describing what you thought about when you reviewed the patch and what you tried.

Thanks!

Here is the new patch. Sorry, i have not attached interdiff since this is a small patch.

@himanshu : your patch doesn't work

git apply -v 2854249-14.patch
2854249-14.patch:20: trailing whitespace.
    $message = ($message ? $message : strtr('Link with label %label not found.', ['%label' => $label]));
2854249-14.patch:29: trailing whitespace.
    $message = ($message ? $message : strtr('No link containing href %href found.', ['%href' => $href]));
Checking patch core/tests/Drupal/Tests/WebAssert.php...
error: while searching for:
   *   Thrown when element doesn't exist, or the link label is a different one.
   */
  public function linkNotExists($label, $message = '') {
    $message = ($message ? $message : strtr('Link with label %label found.', ['%label' => $label]));
    $links = $this->session->getPage()->findAll('named', ['link', $label]);
    $this->assert(empty($links), $message);
  }

error: patch failed: core/tests/Drupal/Tests/WebAssert.php:247
error: core/tests/Drupal/Tests/WebAssert.php: patch does not apply

@krknth Sorry, i assigned it to myself to work on it later. It was because i manually edited the patch, i should have used git diff. @GoZ Thanks it looks good to me but since i also worked on this patch, it's better i don't RTBC it. Thanks for rerolling it. GoZ++

Patch applied successfully and seems OK

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Correcting issue credit. @boaloysius, please don't post any more screenshots of your codebase; they aren't needed. Thanks!

Attributing the contribution to GSoC.