Closed (fixed)
Project:
Drupal core
Version:
9.4.x-dev
Component:
node system
Priority:
Minor
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
15 Apr 2022 at 13:45 UTC
Updated:
9 May 2022 at 13:49 UTC
Jump to comment: Most recent
Problem/Motivation
While working on #3274066: Node tests should not rely on Classy, it was discovered that the test method Drupal\Tests\node\Functional\NodeAdminTest::testAdminFrontPage Drupal\Tests\node\Functional\Views\FrontPageTest::testAdminFrontPage is obsolete.
The test method was added as part of #2191011: PHP Warnings in views_preprocess_page() caused by frontpage view where a bug in views_preprocess_page() was addressed. However, views_preprocess_page() no longer exists.
The hook was changed to views_preprocess_html() in #2352155: Remove HtmlFragment/HtmlPage. The hook existed to do some wild stuff that was no longer needed after #2476947: Convert "title" page element into a block. In that issue, the hook was removed and the js library that the hook attached was removed.
Therefore the test method is obsolete since it covers code that no longer exists.
Proposed resolution
Remove it.
Remaining tasks
Do it.
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
None
Issue fork drupal-3275464
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:
changes
Comments
Comment #2
danflanagan8Setting this as Novice and Minor.
I'm happy to answer questions for new contributor who want to give this one a shot! Thanks!
Comment #5
hebl commentedHi @danflanagan8,
I'm very new to contributing so apologies if I've done this incorrectly. I've made an attempt at finding and removing the obsolete test method. The only method I could find with the name "testAdminFrontPage" was in the Drupal\Tests\node\Functional\Views namespace (see merge request), which is different to what's outlined in the description.
Is there a possibility that it has already been removed from the correct namespace in this file: core/modules/node/tests/src/Functional/NodeAdminTest.php
I couldn't see it in there.
Cheers
Comment #6
danflanagan8@HEBL, thanks for taking on this issue. You are 100% correct about my mistake in the IS. I have corrected the namespace. Nice sleuthing!
The MR looks perfect. I'll wait for the tests to pass and then I'll be able to RTBC it.
In the meantime, I'll set the issue status to "Needs Review". That's the status you want to use when your work is ready and you want to get eyes on it. This is exactly the kind of stuff that we get to learn about on Novice issues. :)
Cheers!
Comment #7
danflanagan8Comment #8
danflanagan8Comment #9
danflanagan8Tests are green. Thanks for taking care of this, @HEBL!
I also fixed another typo in the IS. What was I doing when I wrote that? :)
I'm setting this to RTBC, which means that a committer will eventually take a look at it. This issue is Minor so that may take awhile, but it will happen. Removing the novice tag.
Comment #10
hebl commentedAwesome! Thanks @danflanagan8, appreciate your support.
Comment #12
catchAgreed that not only does this look like the wrong place for that coverage but also that the test just looks obsolete now. Both contextual and views modules have some generic coverage for contextual links.
Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!
Comment #14
catch