Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Issue tags: +Novice

Good idea

patrickd’s picture

Status: Active » Needs review
FileSize
669 bytes

This way?

Status: Needs review » Needs work

The last submitted patch, form_no_lable_class-1419036-3.patch, failed testing.

gnuget’s picture

Status: Needs work » Needs review
FileSize
670 bytes

i just re-attach a new version of the patch with a fix

patrickd’s picture

Status: Needs review » Reviewed & tested by the community

(omg, how could that happen to me x,D)

patch looks good for me :)

andypost’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

That's great but we need a test for this

patrickd’s picture

general test of functionality or simpletest testcase?

sun’s picture

+++ b/core/includes/form.inc
@@ -3957,6 +3957,10 @@ function theme_form_element($variables) {
+  if (!isset($element['#title']) || $element['#title_display'] == 'invisible') {

#title_display can also be 'attribute' and something else (see code lines a bit below this line).

We need to check explicitly for all of them, either the positive ones or the negative ones. I guess that checking for 'before' or 'after' might be the shortest condition.

That said, yes, this needs tests. The test needs to check #title (unset vs. set) and (all) #title_display values.

gnuget’s picture

Oks i changed the condition for check the before and after value on the title_display .

I attached another patch.

I'm going to provide a tests in my next comment.

gnuget’s picture

i created a set of tests for this issue is my first time making tests so i'm sure to this could be done in a better way. I would really appreciate if someone else with more experience can check my tests

Thanks!

kathyh’s picture

Status: Needs work » Needs review

changed to needs review for a quick testbot check

tstoeckler’s picture

Status: Needs review » Needs work

This looks quite good. Some minor issues with the comments though:

+++ b/core/modules/simpletest/tests/form.test
@@ -1628,3 +1628,70 @@ class FormCheckboxTestCase extends DrupalWebTestCase {
+ * Tests adding a .form-label-none css class at the forms with no or no visible <label>

Also below:
css -> CSS

+++ b/core/modules/simpletest/tests/form.test
@@ -1628,3 +1628,70 @@ class FormCheckboxTestCase extends DrupalWebTestCase {
+class FormLabelNoneTestCase extends DrupalWebTestCase {

Could this extend DrupalUnitTestCase instead? It seems like it could...

+++ b/core/modules/simpletest/tests/form.test
@@ -1628,3 +1628,70 @@ class FormCheckboxTestCase extends DrupalWebTestCase {
+    //invisible
...
+    //before
...
+    //after
...
+    //none
...
+    //attribute

I think we could shorten that by doing:

$tests = array(
  'invisible' => TRUE,
  'before' => FALSE,
  ...
);
foreach ($tests as $title_display => $expected) {
 ...
}

Also comments should be prepended with a space and be Sentence case, i.e.
"// None" instead of "//none"

+++ b/core/modules/simpletest/tests/form.test
@@ -1628,3 +1628,70 @@ class FormCheckboxTestCase extends DrupalWebTestCase {
+ * Tests adding a .form-label-none css class at the forms with no or no visible <label>

css -> CSS

+++ b/core/modules/simpletest/tests/form.test
@@ -1628,3 +1628,70 @@ class FormCheckboxTestCase extends DrupalWebTestCase {
+      'name'  =>  'form-label-none css class',

css -> CSS

sun’s picture

Could this extend DrupalUnitTestCase instead? It seems like it could...

Nope. Every test case that relies on hooks or the theme registry (drupal_render()) needs to be a DrupalWebTestCase.

However, this test class can use protected $profile = 'testing'; to speed up the test case setup.

--
Also:

- You may want to assign the returned $output of drupal_render() to $this->drupalSetContent(), and then simply use assertRaw() to check whether the CSS class exists. There's also an assertPattern() helper method for PCREs, but not sure whether that is really necessary here.

- The test should also confirm at least one counter-case; i.e., a condition in which the CSS class does not appear. That is, because it needs to confirm that the condition is not mistakenly triggered always. In fact, every test case that attempts to verify conditional behavior is supposed to do that (but we still have plenty of tests setting a wrong example throughout core).

gnuget’s picture

Assigned: sun » gnuget
Status: Needs work » Needs review
FileSize
2.66 KB

Thanks for all your feedback.

I attach a new and improved version of the patch.

- The test should also confirm at least one counter-case; i.e., a condition in which the CSS class does not appear. That is, because it needs to confirm that the condition is not mistakenly triggered always. In fact, every test case that attempts to verify conditional behavior is supposed to do that (but we still have plenty of tests setting a wrong example throughout core).

In my first tests i added two cases where the CSS class form-label-none not suppose to appear (the case for 'after' and 'before') this time i added another case where the user add a non expected value for the #title_display option, thank you.

scresante’s picture

Status: Needs review » Needs work
Issue tags: +CSS, +Needs tests, +Novice, +markup, +frontend, +TX (Themer Experience)

The last submitted patch, form_no_label_class_with_tests-1419036-14.patch, failed testing.

scresante’s picture

Issue tags: +SprintWeekend2013

The directory structure of the tests has changed drastically.
The second half of the above patch will have to be implemented somewhere within core/modules/system/lib/Drupal/system/Tests/Form/ probably in ElementsLabelsTest.php

scresante’s picture

This is a correction of the first part of the patch and should work cleanly.

The tests still need to be re-written and I'm not clear on how to do that.

scresante’s picture

Status: Needs work » Needs review
Issue tags: +TX (Themer Experience)
LinL’s picture

Assigned: gnuget » Unassigned
Status: Needs review » Needs work

Setting to "needs work" for the tests.

Also unassigning, hope that's OK, gnuget?

oliverhm’s picture

Assigned: Unassigned » oliverhm
Issue summary: View changes
oliverhm’s picture

Assigned: oliverhm » Unassigned
Status: Needs work » Needs review
FileSize
951 bytes

I've created a form and tested the following test scenarios:
- elements with the #title property missing
- elements with the #title property empty
- elements with the #title_display set to hidden

In order to work on all these scenarios I had to change the previous patch, the new patch is attached.

Status: Needs review » Needs work

The last submitted patch, 22: 0001-fixing-issue-1419036.patch, failed testing.

Mschudders’s picture

Status: Needs work » Needs review
gnuget’s picture

Status: Needs review » Needs work

Hi Oliver.

Please check #8 we need to check explicitly for all the possible values in the title_display option.

#18 works but the tests still need to be re-written, so this needs work

InternetDevels’s picture

sun’s picture

Thanks!

  1. We need to add back the test coverage that got lost in the re-roll. The form tests are now located in Drupal\system\Tests\Form\FormTest.
  2. To be consistent with other CSS classes used in Form API, I think the class should be renamed to .form-no-label
  3. To simplify the code and logic, we should move the already existing and subsequently following if (!isset($element['#title'])) condition a few lines up. After doing so, the necessary condition is simply:
    if ($element['#title_display'] != 'before' && $element['#title_display'] != 'after') {
      $attributes['class'][] = 'form-no-label';
    }
    
wheatpenny’s picture

Assigned: Unassigned » wheatpenny

I working on this issue per the post DrupalConNA sprint weekend.

wheatpenny’s picture

Status: Needs review » Needs work
wheatpenny’s picture

Attached patch addresses points 2 and 3 from #27.

wheatpenny’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
wheatpenny’s picture

Assigned: wheatpenny » Unassigned
lauriii’s picture

Status: Needs work » Needs review
FileSize
840 bytes

This should fix the tests

Status: Needs review » Needs work

The last submitted patch, 35: class_to_form_items_without_label-1419036-35.patch, failed testing.

lauriii’s picture

There was still cases that were not covered such as invisible labels. I also applied tests to this patch.

The last submitted patch, 37: class_to_form_items_without_label-1419036-37-tests.patch, failed testing.

wheatpenny’s picture

wheatpenny’s picture

FileSize
67 KB
50.25 KB

I applied the passing patch from #37. According to my testing, the class was added for a field that has no label.

Field with label (no added class):

Screenshot of HTML inspection of field with a label

Field with no label (added "form-no-label" class):

Screenshot of HTML inspection of field without a label

I'm leaving this issue as "Needs review" as I am not familiar enough with testing to comment on the tests in the patch. The tests still need to be reviewed.

lauriii’s picture

Assigned: Unassigned » lauriii
Status: Needs review » Needs work

One line is being deleted in the patch by a mistake. I'll fix that.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
719 bytes

Removed unnecessary line removal from the patch.

lauriii’s picture

Assigned: lauriii » Unassigned
Sir-Arturio’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
255.92 KB
233.68 KB

The patch still adds the form-no-label. Tested both manually (see the attached images) and automatically (with just the test [failed] and test+fix [success]).

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 78a7670 and pushed to 8.x. Thanks!

  • alexpott committed 78a7670 on 8.x
    Issue #1419036 by lauriii, gnuget, wheatpenny, InternetDevels, oliverhm...

Status: Fixed » Closed (fixed)

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