Problem/Motivation

drupalPostForm in functional tests is deprecated

See: https://www.drupal.org/node/3168858

CommentFileSizeAuthor
#25 3175583-25-drupalPostForm.patch67.14 KBtr
#25 interdiff-23-25.txt623 bytestr
#23 3175583-23-drupalPostForm.patch66.69 KBtr
#23 interdiff-21-23.txt1.74 KBtr
#21 3175583-20-drupalPostForm.patch68.39 KBtr
#17 3175583-17.patch71.8 KBPooja Ganjage
#16 3175583-16.patch71.35 KBPooja Ganjage
#15 3175583-15.patch70.73 KBPooja Ganjage
#14 3175583-14.patch70.1 KBPooja Ganjage
#12 3175583-12.patch68.7 KBPooja Ganjage
#9 interdiff_9.txt1.19 KBPooja Ganjage
#9 3175583-9.patch68.65 KBPooja Ganjage
#4 3175583-4.patch68.68 KBPooja Ganjage
#2 3175583-2.patch68.73 KBpaulocs

Comments

paulocs created an issue. See original summary.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Active » Needs review
StatusFileSize
new 3175583-2.patch68.73 KB

A patch for it.

Status: Needs review » Needs work

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

Pooja Ganjage’s picture

Pooja Ganjage commented
StatusFileSize
new 3175583-4.patch68.68 KB

Hi,

I am creating patch for failing #2 comment patch.

Please review the patch.

Thanks.

Pooja Ganjage’s picture

Pooja Ganjage commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: 3175583-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

Hey @Pooja Ganjage,
can you provide the interdiff?

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

SimplenewsSubscribeTest.php is falling even if no patch is applied.

Pooja Ganjage’s picture

Pooja Ganjage commented
StatusFileSize
new 3175583-9.patch68.65 KB
new interdiff_9.txt1.19 KB
Pooja Ganjage’s picture

Pooja Ganjage commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 9: 3175583-9.patch, failed testing. View results

Pooja Ganjage’s picture

Pooja Ganjage commented
StatusFileSize
new 3175583-12.patch68.7 KB
Pooja Ganjage’s picture

Pooja Ganjage commented
Status: Needs work » Needs review
Pooja Ganjage’s picture

Pooja Ganjage commented
StatusFileSize
new 3175583-14.patch70.1 KB
Pooja Ganjage’s picture

Pooja Ganjage commented
StatusFileSize
new 3175583-15.patch70.73 KB
Pooja Ganjage’s picture

Pooja Ganjage commented
StatusFileSize
new 3175583-16.patch71.35 KB
Pooja Ganjage’s picture

Pooja Ganjage commented
StatusFileSize
new 3175583-17.patch71.8 KB
adamps’s picture

adamps commented
Status: Needs review » Needs work

Tests are failing so this is still "needs work"

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: Unassigned » paulocs
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Assigned: paulocs » Unassigned
tr’s picture

tr commented
Status: Needs work » Needs review
StatusFileSize
new 3175583-20-drupalPostForm.patch68.39 KB

Rerolled patch, which should now apply to HEAD, but without the unrelated coding standards fixes (e.g. changes to README.txt) that are in #17. This just changes the drupalPostForm() calls according to the change record drupalPostForm() in functional tests is deprecated

Status: Needs review » Needs work

The last submitted patch, 21: 3175583-20-drupalPostForm.patch, failed testing. View results

tr’s picture

tr commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-21-23.txt1.74 KB
new 3175583-23-drupalPostForm.patch66.69 KB

Test failure seems to be because the patch changed the regular expression in tests/src/Functional/SimplenewsSourceTest.php - I don't know why that was done, but it's clearly outside the scope of this issue. So here's a re-roll with that restored:

adamps’s picture

adamps commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Plan to commit

Looks good from a quick glance thanks. I will review more carefully and commit soon.

tr’s picture

tr commented
StatusFileSize
new interdiff-23-25.txt623 bytes
new 3175583-25-drupalPostForm.patch67.14 KB

Here's a re-roll of #23 to update the patch to remove offsets - other commits since November have altered the line numbers.

adamps’s picture

adamps commented

Thanks @TR sorry I lost track of this one. I notice that you remove a lot of calls to t(). Presumably this is covered by some Drupal coding standard - please can you post a link?

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented

Hello @AdamPS!
Here you can read about it: https://www.drupal.org/docs/automated-testing/phpunit-in-drupal/phpunit-browser-test-tutorial#t

tr’s picture

tr commented

@AdamPS: That is a really, really old "change" to Drupal - see #500866: [META] remove t() from assert message from 2009.

It has taken forever to try eliminate the legacy use of t() in tests in core ... still not finished after all these years because this keeps getting re-introduced ...

The primary purpose of the patch here is not to fix the t() issue, but because t() is used improperly in every drupalPostForm() call in this module my position is that as long as the patch is replacing the drupalPostForm() calls we should ensure we don't make the same mistake of using t() in the new code. I didn't try to fix the t() issue anywhere else in the module tests.

  • AdamPS committed d8a9535 on 3.x authored by TR 
    Issue #3175583 by Pooja Ganjage, TR, paulocs, AdamPS: drupalPostForm in...
adamps’s picture

adamps commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Plan to commit

Thanks that's perfectly clear

Status: Fixed » Closed (fixed)

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