Problem/Motivation

Currently the test checks that text is not present on the page, however it does this as the anonymous user which gets an access denied on this page anyway.https://git.drupalcode.org/project/drupal/-/commit/b2320bc9d58ee9c5df269...

    $this->drupalGet('admin/content/block/' . $loaded->id());
    $this->assertSession()->pageTextNotContains($loaded->body->value);

pageTextNotContains() doesn't make sense here because the page is not available for anonymous user.

Steps to reproduce

NA

Proposed resolution

Login as $this->adminUser in the test.
per @xjm in #22 extend test coverage

Remaining tasks

Implement
Review

User interface changes

NA

API changes

NA

Data model changes

NA

Release notes snippet

NA

CommentFileSizeAuthor
#16 2791061-16.patch1.04 KBsmustgrave
#16 interdiff-6-16.txt1.51 KBsmustgrave
#6 content_revisions_test-2791061-6.patch924 bytesChi
#2 content_revisions_test-2791061-2.patch574 bytesChi

Issue fork drupal-2791061

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chi created an issue. See original summary.

Chi’s picture

Chi CreditAttribution: Chi commented
Status: Active » Needs review
Related issues: +#2716081: BlockContent should have revision_user and revision_created fields and implement RevisionLogInterface
FileSize
content_revisions_test-2791061-2.patch574 bytes

Status: Needs review » Needs work

The last submitted patch, 2: content_revisions_test-2791061-2.patch, failed testing.

Chi’s picture

Chi CreditAttribution: Chi commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Needs work » Needs review
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

assertNoText() doesn't make sense here because the page is not available for anonymous user.

Do you mind adding a $this->assertResponse(200)? I think this would clarify a bit more what is going on, as well as prevented the situation in the first place.

Chi’s picture

Chi CreditAttribution: Chi commented
FileSize
content_revisions_test-2791061-6.patch924 bytes

Added $this->assertResponse(200).

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
FileSize
interdiff-6-16.txt1.51 KB
2791061-16.patch1.04 KB

Still seems valid.

Rerolled

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Issue tags: +Bug Smash Initiative
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

Reran for 10.1

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Needs review » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Agileana commented
Status: Reviewed & tested by the community » Needs work

Read over the whole test and this makes sense. This highlights why it's always a good idea to have positive and negative assertion pairs, rather than just negative assertions.

To that end, could we change the test a bit further? Add a pageTextContains() assertion that asserts what block content should be there at this point in the test?

It'd be even better to place the block, assert its content, change the default revision, and then assert its content.

It'd also be better not to use random machine names for the content of the two revisions, because there is a nonzero chance it could generate the same placeholder and cause a fail.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

@xjm

not sure I follow the points

To that end, could we change the test a bit further? Add a pageTextContains() assertion that asserts what block content should be there at this point in the test?

The block body is nothing when $this->assertSession()->pageTextNotContains($loaded->body->value);
is checked can add a fieldValueEquals if we really need to

It'd be even better to place the block, assert its content, change the default revision, and then assert its content.

Seems to be out of scope for this issue

It'd also be better not to use random machine names for the content of the two revisions, because there is a nonzero chance it could generate the same placeholder and cause a fail.

RandomMachine name is only called once to set the body (which previously was empty) so not sure how there could be conflict?

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Version: 9.5.x-dev » 11.x-dev

smustgrave opened merge request !5917

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Issue summary: View changes
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Issue summary: View changes
Status: Needs work » Needs review
acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

It's not clear to me from the comments or IS why exactly we need to login as the admin user?

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

+1 for logging in as a user with the minimum set of permissions rather than the admin user. Using the admin user masks subtle bugs.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update

Slightly tweaked based on @larowlan comment in #29

Addressed feedback.

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented
Issue summary: View changes
acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented
Issue summary: View changes
Status: Needs review » Needs work

Updated the IS to be more accurate. I'm honestly questioning whether we need this test at all. It's testing the most arbitrary stuff and was first added in a very similar state 10 years ago https://git.drupalcode.org/project/drupal/-/commit/b2320bc9d58ee9c5df269...

What the test does (currently):

  1. Checks the log message set by setRevisionLogMessage in setUp is the same when loading the block revision - Seems redundant.
  2. Checks the revision user, revision id and revision create time are the correct data types, except for the first revision - Very redundant and also exposes a bug that I've reported elsewhere (will find the link soon)
  3. Checks the latest revision is the default revision - Also redundant.
  4. Checks unpublished/non default revision's body text doesn't appear on the frontpage - Possibly useful
  5. Checks the block edit form is a 200 and doesn't contain the latest revision's body text - Redundant and potentially a bug since the edit form SHOULD contain the latest revision's content
  6. Checks the latest revision id is greater than the default revision id - very reundant
  7. Checks the frontpage contains the default revision's body text - possibly useful

When I say redundant I mean it's probably tested elsewhere, or completely useless.

Setting to NW because honestly I reckon we ditch the test entirely after confirming we're covered elsewhere.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

So just checking item 1 this appears to be the only test using setRevisionLogMessage and testing the message on the version-history page.

So not 100% sure it is covered elsewhere.

acbramley’s picture

acbramley CreditAttribution: acbramley at PreviousNext for Service NSW commented

this appears to be the only test using setRevisionLogMessage and testing the message on the version-history page.

But it doesn't check the version history page? It checks getRevisionLogMessage returns what was set in setUp

We do have other testing for the version history page though! https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/block...