As if we wouldn't know that System module is a dumping ground for literally anything, it seems we just went ahead and continued that paradigm with tests.

Attached patch

- moves all tests belonging to common.inc into a new "Common" group.

- fixes various titles and descriptions of affected test cases.

- also attempts to clean up tests in system.test, by prepending "System" to the test case class name if it clearly belongs to System module. However, that's just a simple attempt - properly moving and classifying the test cases in system.test should be a separate issue. The current file contents are a pain.

CommentFileSizeAuthor
#25 drupal8.declutter-system.25.patch13.82 KBsun
#23 declutter-system.patch9.93 KBwebchick
#17 drupal8.declutter-system.17.patch24.27 KBsun
#8 drupal.declutter-system.8.patch25.04 KBsun
drupal.declutter-system.0.patch24.72 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Feedback anyone?

Otherwise, this is pretty much RTBC.

reglogge’s picture

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

Tested the patch and can confirm that it really cleans things up nicely :-)

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

this is a bug report...how?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

How else can you call wrong group assignments and wrong/misleading test case class names?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

drupal.declutter-system.0.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal.declutter-system.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal.declutter-system.8.patch25.04 KB

Re-rolled against HEAD.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

This was RTBC before, just re-rolled against HEAD, so back to RTBC.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#8: drupal.declutter-system.8.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#8: drupal.declutter-system.8.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#8: drupal.declutter-system.8.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#8: drupal.declutter-system.8.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Version: 7.x-dev » 8.x-dev

Sorry, this would invalidate documentation at this point that talks about where to find tests, and we ended the period of being able to make "would be nice" string fixes in December of last year.

Bumping to D8.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

#8: drupal.declutter-system.8.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal.declutter-system.8.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.declutter-system.17.patch24.27 KB

Re-rolled against HEAD.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Assigned: sun » webchick
Status: Needs review » Needs work

Since you gave me a nice review at #1397246: Add drupal_array_unset_nested_value(), I'm yoinking this so I can drive it home for you. :) Hope that's ok.

This'll need a re-roll for /core if nothing else.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue tags: +Novice

Also, this is classic Novice material.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: Declutter "System" test group » [Meta] Declutter "System" test group
Issue tags: -Novice

Ah-hahahaa. No, no it is not. :)

However, I don't think the patch as-is is very committable. It's combining at least the following things:

- Changes the 'System' group to 'XXX' where XXX matches XXX.test.
- Prefixes 'Common', 'System', and so on the name of all test cases, based on which file it belongs to (e.g. common.test, system.test).
- Makes sure all test case names end in 'TestCase'.
- Removes differentiation between "FunctionalTestCase" and "UnitTestCase". Just "TestCase".
- Renames some tests, though I can't quite figure out the pattern on that one. It doesn't seem to be following any recommendations listed in http://drupal.org/node/325974.

Then in going through I also noticed that:
- Lots of files are missing @file declarations
- Lots of files are missing PHPDoc
- Lots of comments don't wrap at 80 chars
- Lots of test cases are named incorrectly or unclearly ("testActionSomething" instead of "testAction*s*Something")

So I sat down to fix these, and quickly it was growing into a gigantic monster patch that would be completely unreviewable, terrifying to commit, and also prone to mass breakage by other patches.

So I think what I'm going to do is turn this one into a "Meta" issue, and then spin off these into separate, actually novice issues. :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Hrm. Except that's not ideal either, because they'll frequently collide with re-rolls.

Ok, I give up. :) I'm going to see if I can track you down in IRC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Title: [Meta] Declutter "System" test group » Declutter "System" test group by applying coding standards fixes to common.test

Talked to sun. We agreed to reduce the scope of this to only common.test, which I fully agree needs a LOT of clean-up. Re-titling.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Needs review
StatusFileSize
new declutter-system.patch9.93 KB

Ok, here's some easy stuff:

- group => 'System' to 'Common'
- prefix all test case classes with 'Common'
- Makes all test case names consistently end in 'TestCase', per standards

I didn't touch the name/description changes, because I couldn't derive a pattern there, and the simpletest coding standards are silent on this point.

Note that this will conflict with #1397246: Add drupal_array_unset_nested_value() so if that one gets in first, this will need a re-roll and vice-versa.

Not sure if my mad find/replace skillz broke anything, so let's see...

Status: Needs review » Needs work

The last submitted patch, declutter-system.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.declutter-system.25.patch13.82 KB

Restored/re-did some oversights. I'm not able to reproduce the test failure of #23.

Looks RTBC to me if it comes back green.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Reviewed & tested by the community

Looks good!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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