Problem/Motivation

This is a follow up to #3134459: Remove @group legacy from migrate tests which removed '@group legacy' from migration tests. This is to do the same for all tests.

They can be found with `grep -r '@group legacy' core` which currently returns 42 lines.

Not sure what component to use so selected 'other'.

Proposed resolution

Remove '@group legacy' from tests where it is no longer needed.

Remaining tasks

Make a patch
Review

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

jyotimishra-developer’s picture

Assigned: Unassigned » jyotimishra-developer
shimpy’s picture

Status: Active » Needs review
FileSize
22.6 KB

Removed @group legacy tags from test

longwave’s picture

Status: Needs review » Needs work

We can't just remove it from all places, we do need the legacy group when we are testing deprecations.

#3135302: Remove Symfony 3 legacy test from ReverseProxyMiddlewareTest covers one of these as well, maybe this should be a meta to review each of these and have child issues for any we find that need to be removed - as in that case some may need more than just the group removing? Not sure.

jyotimishra-developer’s picture

Hi @shimpy, I have assigned this issue to me and was working on it..i feel its unethical.
But anyways..no issues.

jyotimishra-developer’s picture

Removed @group legacy tags from test and corrected errors in patch #3, found in dispatcher result.

jyotimishra-developer’s picture

jyotimishra-developer’s picture

Hardik_Patel_12’s picture

Assigned: jyotimishra-developer » Unassigned
Status: Needs work » Needs review
longwave’s picture

Status: Needs review » Needs work
Related issues: +#3135302: Remove Symfony 3 legacy test from ReverseProxyMiddlewareTest

Thanks for working on this.

  1. +++ b/core/modules/user/tests/src/Unit/Views/Argument/RolesRidTest.php
    @@ -19,8 +19,6 @@ class RolesRidTest extends UnitTestCase {
        * Note this is only a legacy test because it triggers a call to
        * \Drupal\Core\Entity\EntityTypeInterface::getLabelCallback() which is mocked
        * and triggers a deprecation error. Remove when ::getLabelCallback() is
    

    This whole comment can be removed now, because getLabelCallback() has been removed too.

  2. +++ b/core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php
    @@ -114,7 +114,6 @@ protected function setUp(): void {
        * Note the legacy group is used here because
        * ActiveTheme::getStyleSheetsRemove() is called and is deprecated. As this
    

    This comment block can be removed too.

  3. +++ b/core/tests/Drupal/Tests/Core/StackMiddleware/ReverseProxyMiddlewareTest.php
    @@ -88,7 +88,6 @@ public function reverseProxyEnabledProvider() {
    -   * @group legacy
    

    This whole test can be removed, but let's leave this for #3135302: Remove Symfony 3 legacy test from ReverseProxyMiddlewareTest

  4. +++ b/core/tests/Drupal/Tests/Listeners/DrupalStandardsListenerTrait.php
    @@ -180,8 +180,6 @@ public static function errorHandler($type, $msg, $file, $line, $context = []) {
    -   * @group legacy
    -   *
    

    This shouldn't be removed, please read the comment above.

Hardik_Patel_12’s picture

Assigned: Unassigned » Hardik_Patel_12
Hardik_Patel_12’s picture

Assigned: Hardik_Patel_12 » Unassigned
Status: Needs work » Needs review
FileSize
8.88 KB
2.29 KB

@longwave thanks for your review , i have covered point no 1 , 2 and 4 in this patch as mentioned in #10. Point number 3 is leave for #3135302: Remove Symfony 3 legacy test from ReverseProxyMiddlewareTest.

longwave’s picture

Status: Needs review » Needs work

Thanks! Two tiny nitpicks then this is good to go:

  1. +++ b/core/modules/user/tests/src/Unit/Views/Argument/RolesRidTest.php
    @@ -19,12 +19,6 @@ class RolesRidTest extends UnitTestCase {
        * @covers ::titleQuery
        *
    -   * @group legacy
    

    There is an empty comment line at the end of the comment that can now be removed.

  2. +++ b/core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php
    @@ -114,12 +114,7 @@ protected function setUp(): void {
    -   * @group legacy
        *
    -   * Note the legacy group is used here because
    

    There is an empty comment line at the end of the comment that can now be removed.

Hardik_Patel_12’s picture

Status: Needs work » Needs review
FileSize
8.91 KB
532 bytes

Empty comment line are removed from core/modules/user/tests/src/Unit/Views/Argument/RolesRidTest.php and core/tests/Drupal/Tests/Core/Asset/AssetResolverTest.php files.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, this looks perfect now!

xjm’s picture

Saving reviewer and reporter credits.

jyotimishra-developer’s picture

longwave’s picture

Issue tags: +Needs reroll

Needs rerolling.

naresh_bavaskar’s picture

Assigned: Unassigned » naresh_bavaskar
Status: Reviewed & tested by the community » Needs work
Suresh Prabhu Parkala’s picture

Assigned: naresh_bavaskar » Unassigned
Status: Needs work » Needs review
FileSize
8.91 KB

Re-rolled patch please review.

Suresh Prabhu Parkala’s picture

Please review!

longwave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Thanks for rerolling.

#21 looks good, same as #14 with ReverseProxyMiddlewareTest removed as that was committed separately.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 3136389-21.patch, failed testing. View results

longwave’s picture

Status: Needs work » Reviewed & tested by the community

Random fail in WidgetUploadTest, back to RTBC.

xjm’s picture

Title: Remove '@group legacy' from Tests » Remove '@group legacy' from tests that do not exercise deprecated code
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 409af2de16 to 9.1.x and 00798cc725 to 9.0.x. Thanks!

This is an important fix to ensure we're not triggering deprecated code. Nice work everyone.

Backported to 9.0.x as this is a test-only fix.

  • alexpott committed 409af2d on 9.1.x
    Issue #3136389 by jyotimishra123, Hardik_Patel_12, Suresh Prabhu Parkala...

  • alexpott committed 00798cc on 9.0.x
    Issue #3136389 by jyotimishra123, Hardik_Patel_12, Suresh Prabhu Parkala...

Status: Fixed » Closed (fixed)

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