Problem/Motivation

Fixes for the sniff in core/tests

Steps to reproduce

Proposed resolution

Remaining tasks

Review

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 3477205-nr-bot.txt28.95 KBneeds-review-queue-bot

Issue fork drupal-3477205

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
Title: Use DrupalPractice.CodeAnalysis.VariableAnalysis nn core/tests » Use DrupalPractice.CodeAnalysis.VariableAnalysis in core/tests
Status: Needs review » Active

quietone opened merge request !9626

quietone’s picture

quietone commented
Issue summary: View changes
Status: Active » Needs review

Added several phpcs:ignore lines in DriverSpecificTransactionTestBase in order for tests to pass. With a variable for the return the tests fail. I can't say that I understand why.

For \Drupal\Tests\Core\Render\RendererRecursionTest, found this commit may be useful.

smustgrave’s picture

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

Opened #3477815: Investigate need for unused variables in DriverSpecificTransactionTestBase to investigate DriverSpecificTransactionTestBase since this does net gain 40 other files vs holding up.

Reviewed the rest of the changes and cleanup seems good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Needs to be merged with HEAD and I left a small review on the MR.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Rebased with a conflict in phpcs.xml.dist and then accepted the suggestion in the MR.

quietone’s picture

quietone commented

Rebase again due to conflict in \Drupal\Tests\Core\DrupalTest::setMockContainerService

smustgrave’s picture

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

Feedback appears to be addressed.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3477205-nr-bot.txt28.95 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 necessarily 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.

quietone’s picture

quietone commented
Status: Needs work » Needs review

Rebased and then was getting undefined variable, $simpletest_path, so I removed those. That was recently change in #3477529: [CI] Remove the 'with-database' requirement for unit tests.

Back to needs review.

smustgrave’s picture

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

Rebase seems good.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs review

Just one comment, fine to self RTBC after answering/addressing

quietone’s picture

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

@larowlan, thanks.

I explained why it is commented out. and I still think it is the right thing to do here. So, restoring RTBC

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

There are three new fails in HEAD

FILE: ...core/drupal/core/tests/Drupal/Tests/Core/Theme/Icon/IconDefinitionTest.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 225 | WARNING | Unused variable $icon.
     |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable)
--------------------------------------------------------------------------------


FILE: ...rupal/core/tests/Drupal/Tests/Core/Theme/Icon/Plugin/PathExtractorTest.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 128 | WARNING | Unused variable $index.
     |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable)
--------------------------------------------------------------------------------


FILE: ...drupal/core/tests/Drupal/Tests/Core/Theme/Icon/Plugin/SvgExtractorTest.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 132 | WARNING | Unused variable $index.
     |         | (DrupalPractice.CodeAnalysis.VariableAnalysis.UnusedVariable)
--------------------------------------------------------------------------------

Time: 2 mins, 25.99 secs; Memory: 12MB

Fine to self RTBC here

quietone’s picture

quietone commented
Status: Needs work » Needs review

I've made changes to fix the new fails.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Trivial changes, back to RTBC

  • larowlan committed db573665 on 11.x 
    Issue #3477205 by quietone, smustgrave, alexpott: Use DrupalPractice....
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed to 11.x - thanks!

larowlan closed merge request !9626

Status: Fixed » Closed (fixed)

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