Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
The field module uses test class members with underscored names. Some examples are big_user, web_user and admin_user, but there could be others. According to our coding conventions, these should be renamed to bigUser, webUser and adminUser. In addition, some properties are undefined but should be.
See the parent issue #1811638: [meta] Clean-up Test members - ensure property definition and use of camelCase naming convention.
Beta phase evaluation
Issue category | Task, because this is a coding standards change. |
---|---|
Issue priority | Not critical because coding standard changes are not critical. |
Unfrozen changes | Unfrozen because it only changes automated tests. |
Disruption | There is no disruption expected from this sort of change. |
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff.txt | 7.48 KB | rteijeiro |
#40 | clean_up_field_module-2392669-40.patch | 39.92 KB | rteijeiro |
#37 | clean_up_field_module-2392669-37.patch | 39.8 KB | hussainweb |
#37 | interdiff-36-37.txt | 926 bytes | hussainweb |
#36 | clean_up_field_module-2392669-36.patch | 40.07 KB | hussainweb |
Comments
Comment #1
Mile23Removed
DisplayApiTest::field_name
because it's unused.Other than that, I only changed under_score class properties to camelCase, without any other changes.
Comment #4
subhojit777Removed here.
Hence, not changed in this place.
Not removed from here, but renamed.
Converted to camel case due to change.
Why the difference, (1-2) and (3-4).
Comment #5
Mile23Pretty sure I messed that one up. :-)
Let's see how this try goes.
Comment #7
Mile23Woo NetBeans! Time to get rid of it!
Comment #9
Mile23Needed a rebase.
Comment #10
Mile23Comment #11
subhojit777I did
egrep -rl '\$this\->[a-z]+_[a-z_]+' core/modules/field/src/Tests/
and got the output:There were only
$this->web_user
. I was looking intoNumberFieldTest.php
and there was this:Therefore, I made this changes to others too. I think these changes will make sense.
Also I see there are some commented-out codes in
FormTest.php
. Do you think we should remove it?Comment #12
Mile23The fact that you only changed the under_score ones in the interdiff means that those properties are probably not used anywhere else in the class. In that case, just turning them into local variables, or even factoring them away is completely appropriate.
For instance, something like this:
Because that login will persist after
setUp()
is done, and into the test method itself. If the test method needs the property, though, leave it.I'd say commented code is strictly speaking out of scope for this issue, but if not now, when? :-)
Comment #13
subhojit777@Mile23 +1
So shall we change it everywhere else I mean also change it in
NumberFieldTest.php
. I have checked there, it uses$webUser
just for login purpose, making it a class property does not make sense.Comment #14
subhojit777As for the commented codes, I will check whether they can be marked as TODO.
Comment #15
hussainwebJust rerolling for now.
Comment #16
hussainwebBack to NW as per #12.
Comment #17
Mile23Thanks, @hussainweb for rerolling.
@subhojit777 The questions that remain are:
Should we refactor away unused properties? The answer is yes.
Should we mark commented code as @todo? I just made an issue about it: #2411961: Figure out what to do with Drupal\field\Tests\FormTest::testFieldFormMultiple() Add a link to that issue in the @todo.
Comment #18
hussainwebRerolling again... :)
Comment #20
hussainwebFixing the test. I think the other failure was random, something to do with a TWIG class not found.
Comment #21
subhojit777Comment #22
subhojit777Refactor unused properties:
protected $webUser;
fromBooleanFieldtest.php
. Follow #12protected $webUser;
fromNumberFieldTest.php
. Follow #12protected $webUser;
fromEmailFieldTest.php
. Follow #12protected $articleCreator;
fromreEnableModuleFieldTest.php
. Follow #12Comment #23
subhojit777Comment #24
Mile23I'm going to mark this as RTBC even though I have the first patch up there.
It solves the problem of underscore vs. camel case, the patch applies, and my next step is to have the testbot run it again.
Comment #26
alexpottDoes not need to be a property.
Comment #27
AjitSThe patch no longer applies and needs a reroll.
Doing it now.
Comment #28
AjitSLet's see what the testbot says. After it clears, I will make the changes suggested in #26.
Comment #30
AjitSNot sure why the test is failing.
Comment #31
rteijeiro CreditAttribution: rteijeiro commentedRe-rolled!
Comment #33
rteijeiro CreditAttribution: rteijeiro commentedRe-rolled and fixed broken test.
Comment #35
hussainweb@rteijeiro: Might I suggest to include an interdiff for your fixes. I know you rerolled and an interdiff doesn't make sense there but in such situations, I post two different patches. The first just after rerolling and no changes, and second patch after the fixes where you can get an interdiff.
Comment #36
hussainwebFixing the exceptions.
Comment #37
hussainwebI think the change requested in #26 got left out in middle of all the rerolls and failures. Doing that here.
Comment #38
rteijeiro CreditAttribution: rteijeiro commentedLooks great! Good job @hussainweb! Thanks!
Comment #39
alexpottLet's rename this to
$entityTypeId
. Also it's not an array.As above
$entityTypeId
Comment #40
rteijeiro CreditAttribution: rteijeiro commentedNice catch, @alexpott! Thanks!
Comment #41
hussainwebYay! @alexpott: Should be good for RTBC again?
Comment #42
rteijeiro CreditAttribution: rteijeiro commentedBack to RTBC?
Comment #43
alexpottTest code is not frozen in beta. Committed 1f37397 and pushed to 8.0.x. Thanks!