Problem/Motivation

There are no tests of these emails. Discovered working on #2154619: Links broken in new releases email

Steps to reproduce

Proposed resolution

Write the tests

Remaining tasks

Patch
Review
Commit

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#14 3276953-nr-bot.txt1.42 KBneeds-review-queue-bot
#10 3276953-10.patch8.82 KBquietone
#10 interdiff-2-10.txt915 bytesquietone
#2 3276953-2.patch8.82 KBquietone

Issue fork drupal-3276953

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
StatusFileSize
new 3276953-2.patch8.82 KB

Maybe this will sufficient.

quietone’s picture

quietone commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented

Shouldn't these fail until #2154619: Links broken in new releases email is merged?

quietone’s picture

quietone commented

No. This is testing the existing email message.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

In that case test looks good.

Status: Reviewed & tested by the community » Needs work

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

quietone’s picture

quietone commented
Version: 10.0.x-dev » 11.x-dev

Random test failure.

1) Drupal\Tests\layout_builder\FunctionalJavascript\InlineBlockPrivateFilesTest::testPrivateFiles
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "files[settings_block_form_field_file_0]" not found.
quietone’s picture

quietone commented
Status: Needs work » Needs review
quietone’s picture

quietone commented
StatusFileSize
new interdiff-2-10.txt915 bytes
new 3276953-10.patch8.82 KB

Text change needed due to #3182405: Do not use verb "Install" for things other than turning on modules/themes.
The interdiff was made with the option, --colour-words.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Restoring status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3276953-10.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Random failure

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3276953-nr-bot.txt1.42 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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.

rpayanm made their first commit to this issue’s fork.

rpayanm opened merge request !4654

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented

I swapped out withConsecutive() for willReturnOnConsecutiveCalls().

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review
quietone’s picture

quietone commented

Based on the comment at #3306554-24: InvocationMocker::withConsecutive() is deprecated in PHPUnit 9.6 and removed from PHPUnit 10, I don't think that replacing with willReturnOnConsecutiveCalls() is the long term solution.

quietone’s picture

quietone commented

What about using will($this->returnValueMap(? I have updated the MR with that idea, plus some other improvements.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Just copying over comment from the other issue

@longwave

Discussed this again with @mondrake in Slack. We now think that just replacing withConsecutive with willReturnOnConsecutiveCalls is not really the right thing to do - these two are not equivalent, and it just happens that here the return values probably don't matter so the tests happen to pass, but we have not made the correct change.

@quietone's solution appeared to work and believe covers the same as what withConsecutive() did. The other issue used a custom callBack but not sure that applies here.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

New test coverage for something previously untested is always nice. The new code looks good to me and I can't think of anything else to add.

Committed and pushed 4f726e041b to 11.x and beedcb6bde to 10.2.x. Thanks!

  • longwave committed beedcb6b on 10.2.x 
    Issue #3276953 by quietone, rpayanm, smustgrave: Add tests of new...

  • longwave committed 4f726e04 on 11.x 
    Issue #3276953 by quietone, rpayanm, smustgrave: Add tests of new...

Status: Fixed » Closed (fixed)

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

quietone closed merge request !4654