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.

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.

I see where you're coming from, I think the test itself already covers this by expecting exceptions. Each test asserts both the positive and the negative.

Well I decided to do an audit since I was curious and there are some gaps. The second list likely needs addressing here. The third list in a followup.

I decided to check locally actually. 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

WebAssertTest claims coverage, but I cannot trigger a failure

• buttonExists
• selectExists
• optionExists
• buildXPathQuery
• linkExists
• assertEscaped
• responseContains

WebAssertTest does not claim coverage, but probably should cover these.

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

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);
  }

I think we need to address everywhere that we're calling addToAssertionCount. Basically \Drupal\Tests\WebAssert::assert doesn't actually assert anything, but throws an exception if the condition failed. Not sure why it passed before when this was a functional test, probably there is some other assertion involved in setup or drupalGet.

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.

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 📌 Add drupalGet() to KernelTestBase Needs review 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?

Thanks @catch. Re #16

Do we actually need different URLs for the visit calls at all, or could those just use '/' everywhere?

At least testAddressEquals needs the path set, the others could just use / or even NULL or '' probably.

@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?

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.

#19 is spot on. Something Else (TM) is incrementing the assert count in functional tests, but I didn't dig enough to figure out what it was. Might be something in the mink setup or similar. We could probably replicate that behaviour by asserting something in the visit function, but that's a bit sneaky.

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 🐛 WebAssert Tests do not actually test all methods Active

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

Found out why those assert methods that make no assertions are passing in BrowserTestBase tests:

In BrowserTestBase::setUp:

// Ensure that the test is not marked as risky because of no assertions. In
// PHPUnit 6 tests that only make assertions using $this->assertSession()
// can be marked as risky.
$this->addToAssertionCount(1);

I've created another follow up 📌 Deprecate and / or remove unused Controllers and routes from WebAssert tests Active 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.

Updated the docs for ::visit so it's a little more obvious and replaced all the dummy URIs with empty strings. We do still have to call $this->session->visit($uri) which takes a string, so went with empty string over NULL. Would be good to used named arguments for calls to ::visit but I think we try to avoid that. We could split ::visit in to a separate method for each of the params, since I don't think more than one is ever set now, but not sure it's worth it.

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!

mstrelan → closed merge request !8383

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