BlockContentRevisionsTest should use admin account

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

+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.

Slightly tweaked based on @larowlan comment in #29

Addressed feedback.

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.

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.

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...

I've looked through a handful of other entity based tests and can confirm that we're testing this revision API in a lot of other places including Kernel tests around entity_test entities. Block content is nothing special here and I see this as pretty much redundant as explained in #32

acbramley → changed the visibility of the branch 2791061-blockcontentrevisionstest-should-use to hidden.

smustgrave → closed merge request !5917

Well if it was that simple glad I was over complicating it lol. Thanks for doing the research!

OK yeah this test was added in 2013 when custom blocks were originally converted to content entities, that was a before a lot of the entity API, especially around generic revision support, was even in place.

There is probably quite a lot of other redundant test module in block_content module now, but since we have to check for equivalent test coverage each time we remove some, makes sense to go ahead here.

Committed/pushed to 11.x, thanks!

catch → closed merge request !11178

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