Convert WebAssertTest to a Unit test

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This is a massive improvement for this test!

Looks good to me generally, but there was a comment from you in slack in response to a question that I think should be recorded here and addressed.

https://drupal.slack.com/archives/C1BMUQ9U6/p1718924045498369
mstrelan
3 hours ago
so the thing is that this test is not testing that the escaping thing works correctly, its testing that the assert methods work correctly

So in reviewing this I realize this is kind a meta-test. It's a test testing a test class. Normally when there is a bugfix or feature change you need a test only job to prove there is actual coverage. That won't quite work here cause the only thing changed was the test itself so test only would be the same change!

I wonder if we need another branch with these changes but the WebAssert class intentionally broke to prove this version still has the same coverage with the same rules as test only, it should fail.

That being said it looks good to me and consistent, I just don't know if we need the equivalent of the test only job in order to verify.

Well I decided to do an audit since I was curious and there may be a couple of gaps for a followup.
I wanted to learn more about WebAssert anyway.

I cloned your branch and ran the WebAssertTest. All green!
In Web Assert I then would break the function by commenting out the assert or otherwise breaking the logic.

I have confirmed coverage for:

• responseHeaderExists
• responseHeaderDoesNotExist
• pageTextMatchesCount
• buttonNotExists
• linkExistsExact
• linkNotExistsExact
• linkByHrefExists
• linkByHrefExistsExact
• linkByHrefNotExists
• linkByHrefNotExistsExact
• pageTextContainsOnce
• pageContainsNoDuplicateId
• addressNotEquals
• elementTextEquals
• buttonExists

WebAssertTest claims coverage, but I cannot trigger failures

• linkExists
• assertEscaped
• responseContains

WebAssertTest does not claim coverage, but probably should cover these in a followup

• optionExists
• buildXPathQuery
• optionNotExists
• titleEquals
• assert
• fieldDisabled
• fieldEnabled
• hiddenFieldExists
• hiddenFieldNotExists
• hiddenFieldValueEquals
• hiddenFieldValueNotEquals
• statusMessageExists
• statusMessageNotExists
• statusMessageContains
• statusMessageNotContains
• buildStatusMessageSelector
• responseHeaderEquals
• pageTextContains
• fieldValueEquals
• selectExists

Just to expand linkExists is covered by testPipeCharInLocator:

public function testPipeCharInLocator(): void {
    $this->visit('/test-pipe-char', '<a href="http://example.com">foo|bar|baz</a>');
    $this->assertSession()->linkExists('foo|bar|baz');
    $this->addToAssertionCount(1);
  }

Removing the assertion should fail the test if I understand it correctly but does not.

public function linkExists($label, $index = 0, $message = '') {
    $message = ($message ? $message : strtr('Link with label %label not found.', ['%label' => $label]));
    $links = $this->session->getPage()->findAll('named', ['link', $label]);
    $this->assert(!empty($links[$index]), $message);
  }

One final comment for now, I didn't check if I could get failures for the three on HEAD with the same technique, so the tests could have been broken already and not affected by this.

• linkExists
• assertEscaped
• responseContains

I would think that would make fixing those a follow up too, but I can't check that right now.

Edit: crossposted with @mstrelan, I think he likely pinpointed the reason in the comment above.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This looks great and similar-ish to #3390193: Add a drupalGet() method to KernelTestBase although that is for kernel tests and more generic.

I left one (non rhetorical) question on the MR. Do we actually need different URLs for the visit calls at all, or could those just use '/' everywhere?

@catch, also if you agree that the three tests mentioned in 12 can be handled in a follow up we can create those, otherwise they should be address here.

@nicxvan I have to admit I'm not entirely sure what's going on with those three tests - is it that they're false negative failures in HEAD due to assert count being incremented in functional tests unrelated to the assertions we're trying to test?

If that's the case and we're not making it worse, then a follow-up seems fine.

Just realised a huge advantage here is not only the performance improvement (that would have been more than enough for me ;)) but the fact that if we break one of these, we'll know it's because we actually broke it instead of some side effect.

OK yeah that seems like a fatal flaw in the existing test and not a problem for this issue to solve.

Yeah that was the issue, they don't actually test those methods.

I created #3462676: WebAssert Tests do not actually test all methods

I think the only remaining question is whether to remove the controllers you mentioned @mstrelan

I've created another follow up #3465507: Deprecate and / or remove unused Controllers and routes from WebAssert tests I'll do another thorough review, but after discussing in slack I think the controller deprecation is better served in a follow up since there is such an extreme improvement here.

Follow ups have been created for the two issues called out. Code changes look good and tests are green. RTBC

One remaining thing here - I think we should pass an empty string or NULL to ::visit() instead of the path names, so it's clearer that there's no path involved. Otherwise this looks great and agreed with tackling the possibly unnecessary routes/controllers in a follow-up, that's going to need its own round of investigation.

I checked the latest changes and I think all of @catch's requests have been resolved.

Yes looks great.

Committed/pushed to 11.x and backported through to 10.3.x to keep the test coverage in sync, thanks!