Problem/Motivation

Ditto.

Some WebAsserts methods are used to convert to PHPUnit earlier Simpletest tests. The original methods carried $message and $group arguments that were used to print information on the Simpletest Test Results UI - but no longer are relevant in PHPUnit.

Proposed resolution

In order to prevent cruft, deprecate passing more arguments than the signature allows, so that in D10 these deprecation can be converted to exception throwing.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

Status: Active » Needs review
FileSize
10.06 KB

Status: Needs review » Needs work

The last submitted patch, 2: 3160405-2.patch, failed testing. View results

mondrake’s picture

Issue tags: +Deprecated assertions
ravi.shankar’s picture

Assigned: Unassigned » ravi.shankar

I Will work on this.

ravi.shankar’s picture

Assigned: ravi.shankar » Unassigned
Status: Needs work » Needs review
FileSize
13.83 KB
3.76 KB

Here I have added a patch which might fix failed tests of patch #2.

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs change record
+++ b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php
@@ -142,10 +142,10 @@ public function testMultipleClosingBodies_2678662() {
-      ->responseContains(BigPipe::STOP_SIGNAL . "\n\n\n</body></html>", 'The BigPipe stop signal is present just before the closing </body> and </html> tags.');
...
-      ->responseNotContains($js_code_until_closing_body_tag . "\n" . BigPipe::START_SIGNAL, 'The BigPipe start signal does NOT start at the closing </body> tag string in an inline script.');

thank you, in these cases the content of the string removed should be added as a comment, since it is telling somenthing more about the reason the response does or does not contain something.

We also need a change record and deprecation tests to demonstrate that methods called with a $message are yielding a deprecation error.

ravi.shankar’s picture

I have added string as a comment as suggested in comment #7.

It still needs work for remaining things of comment #7.

mondrake’s picture

Fixing one test, and adding the same check also to methods inherited from Mink's base WebAssert class.

mondrake’s picture

mondrake’s picture

mondrake’s picture

Closing #3143398: WebAssert methods do not have a $message argument like AssertLegacyTrait methods as duplicate of this - please give @cburshcka credit for his work on the other issue

jungle’s picture

Issue summary: View changes

This is great, thanks @mondrake for filing this.

  1. I would suggest adding a dynamic one to $dynamic_skipped_deprecations in \Drupal\Tests\Listeners\DeprecationListenerTrait::isDeprecationSkipped()
  2. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -642,4 +694,354 @@ public function pageContainsNoDuplicateId() {
    +  public function elementsCount($selectorType, $selector, $count, ElementInterface $container = null) {
    ...
    +  public function elementExists($selectorType, $selector, ElementInterface $container = null) {
    ...
    +  public function elementNotExists($selectorType, $selector, ElementInterface $container = null) {
    ...
    +  public function fieldExists($field, TraversableElement $container = null) {
    ...
    +  public function fieldNotExists($field, TraversableElement $container = null) {
    ...
    +  public function fieldValueEquals($field, $value, TraversableElement $container = null) {
    ...
    +  public function fieldValueNotEquals($field, $value, TraversableElement $container = null) {
    ...
    +  public function checkboxChecked($field, TraversableElement $container = null) {
    ...
    +  public function checkboxNotChecked($field, TraversableElement $container = null) {
    

    Should be $container = NULL

  3. Applied issue template
mondrake’s picture

Category: Task » Bug report
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative
FileSize
23.15 KB
47.42 KB

Thanks @jungle!

#13.1 - actually we should not silence, but remove cruft
#13.2 - done, thanks

Changed from task to bug and tagged 'Bug Smash Initiative' as suggested on Slack.

mondrake’s picture

#3133355: Introduce PHPUnit-compliant WebAssert::responseHeaderExists() method, and its negative introduces a WebAssertTest class that would be useful to then add the deprecation tests to. That's been in RTBC for a while, so let's see if that goes in soon rather than add here too.

jungle’s picture

Status: Needs review » Needs work

Thanks @mondrake!

  1. #13.1 - actually we should not silence, but remove cruft

    You are RIGHT!

  2. Should we add @legacy tests testing deprecated messages?

  3. +++ b/core/modules/path/tests/src/Functional/PathAliasTest.php
    @@ -414,7 +414,7 @@ public function testDuplicateNodeAlias() {
    +    $this->assertSession()->pageTextContains(t('1 error has been found: URL alias'));
    

    Looks like t() call could be removed.

  4. +++ b/core/tests/Drupal/Tests/WebAssert.php
    @@ -70,6 +71,9 @@ protected function cleanUrl($url) {
    +      @trigger_error('Calling ' . __METHOD__ . ' with more than two arguments is deprecated in drupal:9.1.0 and will throw an exception in drupal:10.0.0. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
    
    ...
    
    @@ -92,6 +96,9 @@ public function buttonExists($button, TraversableElement $container = NULL) {
    +      @trigger_error('Calling ' . __METHOD__ . ' with more than two arguments is deprecated in drupal:9.1.0 and will throw an exception in drupal:10.0.0. See https://www.drupal.org/node/TODO', E_USER_DEPRECATED);
    
    

    CR needed/CR URL to be updated, setting back to NW for this at least.

mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
FileSize
48.38 KB
37.67 KB

Thanks @jungle

#16.3 - done
#16.4 - done

Tests still outstanding. Thinking whether to wait for #15, or write a new unit test that mocks a response - 52 functional tests just for deprecation are a performance hit.

Status: Needs review » Needs work

The last submitted patch, 17: 3160405-17.patch, failed testing. View results

ravi.shankar’s picture

Status: Needs work » Needs review

Changin status to needs review as patch #17 passed the tests.

longwave’s picture

Status: Needs review » Needs work

#3133355: Introduce PHPUnit-compliant WebAssert::responseHeaderExists() method, and its negative landed, so we can add to WebAssertTest, but also not sure about adding 52 new functional tests just to cover these deprecations.

mondrake’s picture

mondrake’s picture

Status: Needs work » Needs review
FileSize
56.11 KB
7.74 KB

Re. #17 and #20, here adding a unit test that mocks the session, so tests can be run much faster, and 52 of them would not be a burden... but to write them. Just done a few methods, will need work for the rest.

mondrake’s picture

Status: Needs review » Needs work
mondrake’s picture

Some progress. The complicated part is to mock the DOM elements for methods that do xpath queries. I haven't come up with a solution yet.

mondrake’s picture

FileSize
21.13 KB
mondrake’s picture

Found a way, some progress but more to go.

mondrake’s picture

Assigned: Unassigned » mondrake

On this.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
77.17 KB
20.79 KB

Completed adding tests, ready for review.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Nice work on the unit tests for this! I reviewed all the changes and don't see any issues here.

+++ b/core/modules/path/tests/src/Functional/PathAliasTest.php
@@ -414,7 +414,7 @@ public function testDuplicateNodeAlias() {
-    $this->assertSession()->pageTextContains(t('1 error has been found: URL alias'), 'Form error found with expected text.');
+    $this->assertSession()->pageTextContains('1 error has been found: URL alias');

Great that we are fixing these, this was surely an incorrect use of t() anyway.

  • larowlan committed 050b91b on 9.1.x
    Issue #3160405 by mondrake, ravi.shankar, jungle, longwave: Deprecate...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

This is some top-shelf work folks - I love that we both fix it and prevent it happening further

Committed 050b91b and pushed to 9.1.x. Thanks!

Published the change record.

TR’s picture

Would you please list in the CR which WebAssert methods are affected by this, so a search of the change records for a specific deprecated method will reveal this change and so we have guidance on what we need to change to be D9.1-compatible? Yeah, I guess I could figure it out by myself on a case-by-case basis when this change affects one of my projects, but it would be much easier if there were a list so I could just grep to see if this is even an issue for my projects and so I can fix all cases at the same time.

mondrake’s picture

#32 updated the CR with the list of affected methods.

Status: Fixed » Closed (fixed)

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