Problem/Motivation

Tests for Drupal\Core\Template\Attribute are split between the Common and Template test namespaces. This makes related test coverage harder to locate.

Steps to reproduce

Review Drupal\Tests\Core\Common\AttributesTest and Drupal\Tests\Core\Template\AttributeTest. Both classes test Drupal\Core\Template\Attribute.

Proposed resolution

Move the tests and data provider from Drupal\Tests\Core\Common\AttributesTest into Drupal\Tests\Core\Template\AttributeTest. Remove the empty Common test class.

Remaining tasks

Review the merge request and confirm the consolidated test class passes.

User interface changes

None.

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

Moved the AttributeTest class tests from Common to Template.

Issue fork drupal-2850635

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

Aki Tendo created an issue. See original summary.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Component: base system » theme system
Issue tags: -testing

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now 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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). 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.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now 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: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

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

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now 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.

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

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now 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.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Active » Postponed (maintainer needs more info)
Issue tags: +stale-issue-cleanup

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave’s picture

smustgrave commented
Assigned: Aki Tendo » Unassigned
Status: Postponed (maintainer needs more info) » Active
Issue tags: +Novice, +Needs issue summary update

Probably a good novice task for sure.

kieran.cott made their first commit to this issue’s fork.

kieran.cott opened merge request !16734

kieran.cott’s picture

kieran.cott
Location Manila 🇵🇭
commented
Issue summary: View changes
Status: Active » Needs review

I've updated the issue summary and opened an MR that moves tests in Drupal\Tests\Core\Common\AttributesTest to Drupal\Tests\Core\Template\AttributeTest and removed the empty Common test class.

kieran.cott changed the visibility of the branch 2850635-merge-drupaltestscorecommonattributestest-and to hidden.

dcam’s picture

dcam commented
Status: Needs review » Needs work

This needs more work than to copy the functions of one test into the other. There is duplication of tests between the two.

testAttributeIteration() appears to have the exact same purpose as testIterate(). The difference is that testIterate() tests more iterations, which would make testAttributeIteration() superfluous and not worth copying.

testDrupalAttributes() looks identical in purpose to testAttributeValues(). I didn't study them in-depth, but it looks like most of the test cases between the two functions are valid, with maybe one or two being duplicative. Instead of copying testDrupalAttributes(), I'd merge the cases from providerTestAttributeData() into providerTestAttributeValues().

kieran.cott’s picture

kieran.cott
Location Manila 🇵🇭
commented
Status: Needs work » Needs review

Thanks, @dcam, I've updated AttributeTest.php to:
- Remove redundant testAttributeIteration().
- Remove testDrupalAttributes() and its provider.
- Merge all distinct cases into providerTestAttributeValues() as named datasets.

dcam’s picture

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

Much better. Thank you for doing that additional work.

Committers: The comments from the body of providerTestAttributeData() were not copied to providerTestAttributeValues(). I'm not sure that they're necessary, given that each test case has a key. But there's probably a little bit of context lost. Please check it when reviewing.

mstrelan’s picture

mstrelan commented

Has anyone investigated why we have two similar tests in the first place? Looks to me like the one in Core\Common namespace was in #2003568: Convert tags,attributes, diff and url validation unit tests to phpunit and the Core\Template one was a few months later, in #2083941: \Drupal\Core\Theme\Attribute->value() is named wrong and does not work. Seems like it was not intentional, just missed that a test already exists.

After this there are only 2 tests left in that Core\Common namespace. I wonder if there is a better place for them? Not in scope for this issue.

EDIT: opened #3618406: Move DiffArrayTest and TagsTest to Core\Utility namespace

dcam’s picture

dcam commented

Has anyone investigated why we have two similar tests in the first place?

I didn't check.

dcam’s picture

dcam commented

If the tests were committed a few months apart, then it's entirely possible they were developed in parallel.

  • mstrelan committed f5723cd5 on 11.x 
    task: #2850635 Merge \Drupal\Tests\Core\Common\AttributesTest and \...

  • mstrelan committed e31610eb on main 
    task: #2850635 Merge \Drupal\Tests\Core\Common\AttributesTest and \...
mstrelan’s picture

mstrelan commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

This didn't cherry-pick cleanly since #3562361: Add type hints to core/tests code via Rector - round 2 was not backported to 11.x, but since it was only adding types to a now-deleted file I resolved it by deleting the same file in the cherry-pick commit.

Committed and pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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