Problem/Motivation

Form API's integration with javascript through #states is extremely complex. Now we have JavascriptTestBase we should test this.

Proposed resolution

Add copious tests and fix bugs found.

We can get inspiration for tests from:

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#59 add_javascript_tests-2702233-59.patch25.95 KBdagmar
#58 interdiff-2702233-56-58.txt8.59 KBdagmar
#58 add_javascript_tests-2702233-58.patch8.59 KBdagmar
#57 add_javascript_tests-2702233-56.patch21.99 KBdagmar
#56 interdiff-2702233-52-56.txt3.77 KBdagmar
#56 add_javascript_tests-2702233-56.txt21.99 KBdagmar
#52 interdiff.txt489 bytesManuel Garcia
#52 add_javascript_tests-2702233-52.patch21.93 KBManuel Garcia
#48 add_javascript_tests-2702233-48.patch21.94 KBjibran
#48 interdiff.txt17.39 KBjibran
#2 2702233-2.patch3.96 KBalexpott
#3 2702233-3.patch3.99 KBalexpott
#3 2-3-interdiff.txt1.48 KBalexpott
#6 2702233-6.patch14.9 KBalexpott
#6 3-6-interdiff.txt12.86 KBalexpott
#8 6-8-interdiff.txt2.52 KBalexpott
#8 2702233-8.patch15.89 KBalexpott
#12 interdiff.txt6.11 KBjibran
#12 add_javascript_tests-2702233-12.patch17.23 KBjibran
#19 interdiff.txt6.02 KBjibran
#19 add_javascript_tests-2702233-19.patch16.35 KBjibran
#21 interdiff.txt34.56 KBjibran
#21 add_javascript_tests-2702233-21.patch29.32 KBjibran
#28 interdiff.txt19.33 KBjibran
#28 add_javascript_tests-2702233-28.patch21.72 KBjibran
#31 add_javascript_tests-2702233-31.patch21.65 KBjibran
#36 add_javascript_tests-2702233-36.patch15.47 KBDuaelFr
#40 add_javascript_tests-2702233-40.patch18.41 KBDuaelFr
#40 interdiff.2702233.36.40.txt19.62 KBDuaelFr
#45 interdiff.2702233.40.45.txt4.78 KBDuaelFr
#45 add_javascript_tests-2702233-45.patch17.67 KBDuaelFr
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
3.96 KB

Test looks like it has found a bug :(

alexpott’s picture

Hmmm maybe this is not a bug because this is fixable with a default value... but it does suggest that there is interesting behaviour due to order different things happen when processing #states.

dawehner’s picture

Nice!

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
@@ -0,0 +1,48 @@
+    // This should make the element visible but it is already.
+    $driver->check($xpath);
+    $this->assertElementNotVisible('#edit-name', 'Name field is not visible.');

That comment is a bit weird.

alexpott’s picture

Status: Needs work » Needs review
FileSize
14.9 KB
12.86 KB

Expanded the test using the form from the Drupal 7 examples module (@xjm's idea). And fixed the comments @dawehner mentions in #5

larowlan’s picture

  1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,110 @@
    +    $this->assertElementVisible('#edit-name', 'Name field is not visible.');
    

    not visible » visible

  2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,110 @@
    +//    $driver->setValue(CssSelector::toXPath('#edit-more-info'), '');
    +//    $this->assertFalse($driver->isChecked(CssSelector::toXPath('#edit-info-provide')), '#edit-info-provide is not checked');
    

    Did you try set value NULL or FALSE?

alexpott’s picture

@larowlan thanks for the review:

  1. Fixed by adding a sensible default message to the asserts. Which is a good thing because these are using assertTrue and assertFalse
  2. Yep - in fact if you just open the page and change the value using jQuery it does not work so maybe we have some improvements we can make to states.js
dawehner’s picture

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
@@ -0,0 +1,110 @@
+  public function testJavascriptStates() {

I'm wondering how we could organize the test to have more of a structured approach to all the different kind of cases of tests

xjm’s picture

I'm wondering how we could organize the test to have more of a structured approach to all the different kind of cases of tests

Yeah, I wondered the same thing while reading the patch.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
@@ -0,0 +1,110 @@
+ * Tests if we can execute JavaScript in the browser.

Probably wants an actual docblock. :)

jibran’s picture

  • Fixed the failing tests. Pro tip: some time we have to wait for browser to do its work /HT: @larowlan
  • Updated the docs.
  • Updated the usage of a deprecated CssSelector.

Can we move #9 to follow up it is not really this issues problem we can discuss it in dedicated issue?

jibran’s picture

  1. +++ b/core/includes/common.inc
    @@ -568,7 +568,7 @@ function drupal_js_defaults($data = NULL) {
    - * @see form_example_states_form()
    

    This doesn't exist in core not even in D7. I don't know why we added this ref in #767738: Rewrite drupal_process_states() documentation, deal with action of 'required' for #states.

  2. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -44,6 +45,7 @@
    +  use AssertContentTrait;
    

    I don't think this change is out of scope here.

  3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -92,12 +86,13 @@ public function testJavascriptStates() {
    +    $this->assertJsCondition('!jQuery("#edit-more-info").val()');
    

    This is normal because browser are always slow to react. For behat we are used to of adding wait.

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -92,12 +86,13 @@ public function testJavascriptStates() {
         // This is not working for some reason.
    

    Looks like it is now.

  2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -92,12 +86,13 @@ public function testJavascriptStates() {
    +    $this->assertJsCondition('!jQuery("#edit-more-info").val()');
    

    Is this really necessary? I don't think you have to wait for this... if waiting is necessary then we need to wait for #edit-info-provide to be unchecked.

  3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -20,6 +20,7 @@
    +use Drupal\simpletest\AssertContentTrait;
    
    @@ -44,6 +45,7 @@
    +  use AssertContentTrait;
    

    This seems like a big change and unrelated to the patch - should be a separate issue.

  4. Imo #9 is totally within the scope of this patch
alexpott’s picture

Also AssertContentTrait adds a tonne of methods that just don;t work on BrowserTestBase ie assertText...

The last submitted patch, 12: add_javascript_tests-2702233-12.patch, failed testing.

The last submitted patch, 12: add_javascript_tests-2702233-12.patch, failed testing.

dawehner’s picture

Is this really necessary? I don't think you have to wait for this... if waiting is necessary then we need to wait for #edit-info-provide to be unchecked.

Well, the interaction is not instant but async, but I cannot imagine that for states we ever do the communication with phantomjs fast enough, compared to phantomjs unchecking/checking a checkbox.

jibran’s picture

Thanks @alexpott for the review. From #14

  • Fixed.
  • Removed.
  • Removed.
  • This is a green patch so feel free to bikeshed on it. :D

In this patch

  • Used page instead of driver. I think this makes more sense because of better error handling.
  • Used fillField, uncheckField and checkField because these show what we are doing on the page.
  • Used WebAssert instead of simple true false asserts because of better error handling.
  • Empty and filled states are checked on keyup event and MinkDriver doesn't trigger keyup when the fields are cleared so I manually triggered that and fixed the fail.
dawehner’s picture

  1. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
    @@ -0,0 +1,273 @@
    +    $form['name'] = array(
    

    Do you mind using short array syntax?

  2. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
    @@ -0,0 +1,273 @@
    +        'high_school'   => $this->t('High School'),
    +        'undergraduate' => $this->t('Undergraduate'),
    +        'graduate'      => $this->t('Graduate'),
    

    Can we avoid this indentation? Its just weird ...

  3. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,105 @@
    +    // High school form testing.
    +    $this->click('#edit-student-type-high-school');
    +    $this->assertElementVisible('#edit-high-school');
    +    $this->assertElementNotVisible('#edit-undergraduate');
    +    $this->assertElementNotVisible('#edit-graduate');
    +    $this->assertElementVisible('#edit-average');
    

    So this is the thing, while this is a nice example from the example module, it makes it hard to follow the test. In case we would name things after what we test there, it would be possible to actually read the test without figuring the entire form out first. Just for example take 'Feedback testing', this comment doesn't help you at all of what we test, or right we test a textarea.

jibran’s picture

Thanks for awesome review @dawehner.

  1. Fixed.
  2. Fixed.
  3. Well this was a huge task but I was able to convert it in a readable format. I have to add methods to \Drupal\simpletest\WebAssert but now this is perfectly readable imo. I don't know how much of this is out of scope here but I have updated \Drupal\Tests\toolbar\FunctionalJavascript\ToolbarIntegrationTest as well and cleaned up BTB.

In this patch

  • Added WebAssert::assertFieldVisible(), WebAssert::assertFieldNotVisible(), WebAssert::assertElementVisible() and WebAssert::assertElementNotVisible() to \Drupal\simpletest\WebAssert
  • Fixed doc issues in JavascriptStatesForm
  • Modernized the ToolbarIntegrationTest
  • Fixed doc issues in JavascriptStatesTest
  • Removed the obsolete methods form JavascriptTestBase
  • Updated JavascriptTestBase::assertJsCondition() to use session wait function instead of driver.
  • Removed the obsolete methods form BrowserTestBase
jibran’s picture

FWIW I used \Behat\MinkExtension\Context\MinkContext as a reference for these changes.

alexpott’s picture

@jibran I think @dawehner didn't mean that. I think he meant the labels on the form. Things like 'High School', etc... don't tell us what we're testing.

Status: Needs review » Needs work

The last submitted patch, 21: add_javascript_tests-2702233-21.patch, failed testing.

dawehner’s picture

@jibran
I'm sorry but I fear you didn't got my point. My point was that the test form is not really explicit about what its testing about. For example the feedback part of the form, is about testing 'textarea' states, so it should be named afterward

On top of that we should avoid putting too many custom stuff into your testing framework, but rather use what mink already provides for us, see https://github.com/Behat/Mink/blob/master/src/Behat/Mink/WebAssert.php
So yeah I think we should have an issue about using that. Also additional cleanup you were doing IMHO belongs into its own issue.

dawehner’s picture

Issue tags: +DrupalCampES
larowlan’s picture

What I love about this test/these changes are the readability.

We are nearing the fluency of cucumber, I would argue a non-technical user could get a fair idea of what was being tested. And we're using domain language - there are very few css selectors left, which carry no meaning.

This is the first big javascript test in core, and also one of the biggest BTB new generation tests.

I think it is important to set the standard/bar high in terms of understanding.

I would bet that anyone new to testing would much prefer the format of the states test to your average simpletest.

Great work here @jibran

  1. +++ b/core/modules/simpletest/src/WebAssert.php
    @@ -52,16 +56,149 @@ public function buttonExists($button, TraversableElement $container = NULL) {
    +  public function assertFieldVisible($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldNotVisible($locator, TraversableElement $container = NULL) {
    

    I think there is merit in assertField{Not}VisibleByName methods - would simplify the tests below (lots of 'named' calls). Ok for follow up

  2. +++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php
    @@ -23,30 +23,29 @@ public function testToolbarToggling() {
    +    $content = $page->findLink('Content');
    +    $this->assertTrue($content->isVisible(), 'Toolbar tray is open by default.');
    +    $page->clickLink('Manage');
    +    $this->assertFalse($content->isVisible(), 'Toolbar tray is closed after clicking the "Manage" link.');
    +    $page->clickLink('Manage');
    +    $this->assertTrue($content->isVisible(), 'Toolbar tray is visible again after clicking the "Manage" button a second time.');
    

    This reads much better now

  3. +++ b/core/modules/toolbar/tests/src/FunctionalJavascript/ToolbarIntegrationTest.php
    @@ -23,30 +23,29 @@ public function testToolbarToggling() {
    +    $page->pressButton('Vertical orientation');
    ...
    -    $this->click('#toolbar-item-administration-tray button.toolbar-icon-toggle-vertical');
    

    <3 so much better

  4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,109 @@
    +    $this->assertSession()->assertFieldNotVisible('Name');
    

    you could put $this->assertSession() in a local variable, and save the myriad of calls.

  5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,109 @@
    +    $this->assertSession()->assertElementNotVisible('named', ['fieldset', 'High School Information']);
    

    Merit in a assertFieldsetIsVisible method? follow up ok

  6. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,109 @@
    +    $page->selectFieldOption('How many years have you completed?', '5');
    ...
    +    $page->findById('edit-student-type-graduate')->selectOption('graduate');
    

    selectFieldOption reads much nicer than findById->selectOption

  7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,109 @@
    +    $page->findField('Please describe your graduate studies')->keyUp(8);
    

    We should use a constant here

jibran’s picture

Modernized the ToolbarIntegrationTest

Created #2711963: Modernized ToolbarIntegrationTest for that.

@jibran I think @dawehner didn't mean that. I think he meant the labels on the form. Things like 'High School', etc... don't tell us what we're testing.

@jibran
I'm sorry but I fear you didn't got my point. My point was that the test form is not really explicit about what its testing about. For example the feedback part of the form, is about testing 'textarea' states, so it should be named afterward

Ok, I completely misunderstood that point :D

On top of that we should avoid putting too many custom stuff into your testing framework

I disagree. If we ever want to fix #1567500: [meta] Pave the way to replace the testing framework with PHPUnit and possibly rewrite Simpletest then we have to bring BTB/JTB more inline with our simpletest kind of asserts. I think we should add wrapper functions in \Drupal\simpletest\WebAssert so that we can have feature parity with WTB. And as I said before in #22 these changes exit in MinkContext and DrupalContext so this is not totally new stuff for regular behat users.

@larowlan thank for the review.

  1. Let's create a follow up and keep the discussion alive in new issue.
  2. Yeah, I know. Moved to #2711963: Modernized ToolbarIntegrationTest.
  3. Oh yeah!
  4. Done.
  5. Yeah follow up is fine.
  6. Fixed.
  7. FIxed.

Lee also suggested we should move \Drupal\simpletest\WebAssert out of simpletest \Drupal\simpletest\BrowserTestBase.
In this patch

  • Cleaned up \Drupal\simpletest\WebAssert.
  • Added new methods to \Drupal\simpletest\WebAssert.
  • Reverted changes of ToolbarIntegrationTest.
  • Reverted JavascriptTestBase.
  • Reverted BrowserTestBase.
jibran’s picture

Status: Needs work » Needs review
Wim Leers’s picture

On top of that we should avoid putting too many custom stuff into your testing framework, but rather use what mink already provides for us

+1

A significant reason why SimpleTest is painful to use, is because there's dozens and dozens and dozens of assert methods. Too many to chose from.

I think it'd be better to initially have all these new assert methods be in this particular test class. Once we see the need arise elsewhere, we can choose to put it in its own trait. And eventually we could chose to move it into Drupal's WebAssert itself.

i.e., at some time in the future:

  1. +++ b/core/modules/simpletest/src/WebAssert.php
    @@ -52,16 +56,152 @@ public function buttonExists($button, TraversableElement $container = NULL) {
    +  public function assertFieldVisible($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldNotVisible($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldsetVisible($locator, TraversableElement $container = NULL) {
    ...
    +  public function assertFieldsetNotVisible($locator, TraversableElement $container = NULL) {
    

    I think these could fit in a FormWebAssert — many tests don't have forms, and these are just distracting.

  2. +++ b/core/modules/simpletest/src/WebAssert.php
    @@ -52,16 +56,152 @@ public function buttonExists($button, TraversableElement $container = NULL) {
    +  public function assertElementVisible($selector_type, $selector, ElementInterface $container = NULL) {
    ...
    +  public function assertElementNotVisible($selector_type, $selector, ElementInterface $container = NULL) {
    ...
    +  protected function assertElement($condition, $message, Element $element) {
    

    These feel more universally useful and could potentially be moved into WebAssert at some point.

    But… I'm not even sure we really need these. It feels like overengineering at this time. Why can't these just use Mink's assertion methods directly?

DuaelFr’s picture

Related issues: +#2708823: Test form #states

I had started to work on this on another issue (didn't find this one, thanks @dawehner for closing as duplicate) : #2708823: Test form #states

I was thinking about testing all states on all field types because I remember that some of them behave unexpectedly (see #994360: #states cannot disable/enable radios and checkboxes).
Even if I didn't progress a lot, I found out that checkboxes, managed_file, password_confirm and radios elements did not work as expected with some common states (optional/required, visible/invisible and enabled/disabled).

That JS code is highly dependant of the html structure of the fields, so I really think we should test all combinations on all elements.

dawehner’s picture

We should try to keep this running again :)

Lendude’s picture

Status: Needs review » Needs work
Issue tags: +DevDaysMilan

Discussed moving this forward with @dawehner and @DuaelFr at DevDays.

We agreed that for this to move forward we need to address the feedback from @dawehner in #20/#23/#25 about the form elements needing to be more descriptive in what they are there to test for ('textarea_dependent_on_selector', along those lines), instead of having a nice real world example.

We also agreed that currently we don't want the extra assertion methods (as per @dawehner in #25 and @Wim Leers in #30), but the patch should look more along the lines of how it did in #19. We felt it's better to just get some javascript tests in and later decide if certain patterns are evident in the existing tests and only then add additional assertion methods, over adding methods that might not be used but we can't remove anymore because they might be used by contrib.
Easier to add things later then removing them and having to deal with BC stuff.

@DuaelFr said he was going to work on this, so feedback on these ramblings would be great and awesome work so far!

DuaelFr’s picture

I'm working on this.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
15.47 KB

Here is my progress.
I'd need some help to figure out why the last assertions are failing. (See the @todo comment)
I'd also need to be comforted about the fact I didn't lost myself in translation too ;)

Status: Needs review » Needs work

The last submitted patch, 36: add_javascript_tests-2702233-36.patch, failed testing.

DuaelFr’s picture

I discussed about this patch with @Lendude this morning (thanks!).
I'm going to rewrite it to make it more understandable.
New patch coming soon.

jibran’s picture

As we are seeing in #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) we need a lot of new/old assert methods so I still think we should keep the new assert methods.

+++ b/core/modules/simpletest/src/WebAssert.php
@@ -52,16 +56,152 @@ public function buttonExists($button, TraversableElement $container = NULL) {
+  public function assertFieldVisible($locator, TraversableElement $container = NULL) {
...
+  public function assertFieldNotVisible($locator, TraversableElement $container = NULL) {
...
+  public function assertFieldsetVisible($locator, TraversableElement $container = NULL) {
...
+  public function assertFieldsetNotVisible($locator, TraversableElement $container = NULL) {
...
+  public function assertElementVisible($selector_type, $selector, ElementInterface $container = NULL) {
...
+  public function assertElementNotVisible($selector_type, $selector, ElementInterface $container = NULL) {
...
+  protected function assertElement($condition, $message, Element $element) {
...
+  protected function getMatchingElementRepresentation($selector_type, $selector, $plural = FALSE) {

These all can now go into JsWebAssert or a AssertFieldTrait or AssertJSFieldTrait and then only this test will have to use the trait.

I'm going to rewrite it to make it more understandable.

I don't think this is necessary at all If you can't make sense of the form then just enable the module and visit the page. Test will be more understandable if we'll add new assert methods.

IMO this patch is now going in a wrong direction. I still think #31 is the way forward but if no one agrees with me then I'll keep mum about it.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
18.41 KB
19.62 KB
I'm going to rewrite it to make it more understandable.

I don't think this is necessary at all If you can't make sense of the form then just enable the module and visit the page. Test will be more understandable if we'll add new assert methods.

Too late. Here is my patch.

@Lendude and @dawehner agreed it was too early to add a bunch of new assertions as, for tests, we are more in a mood where we do things then, when we'll have enough tests, we'll think about refactoring. Providing many assertions right now means they are going to be there forever because we won't want to break contrib tests that should use them.

jibran’s picture

  1. +++ b/core/modules/system/tests/modules/form_test/src/Form/JavascriptStatesForm.php
    @@ -0,0 +1,183 @@
    +  public function buildForm(array $form, FormStateInterface $form_state) {
    

    This form is understandable now but not intuitive imho.

  2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Form/JavascriptStatesTest.php
    @@ -0,0 +1,260 @@
    +    $this->_testCheckboxTriggeredElements($page);
    +    $this->_testTextfieldTriggeredElements($page);
    +    $this->_testRadiosTriggeredElements($page);
    +    $this->_testCheckboxesTriggeredElements($page);
    +    $this->_testSelectTriggeredElements($page);
    +    $this->_testMultipleTriggeredElements($page);
    ...
    +    $this->assertTrue($page->findField('textfield_invisible_when_checkbox_trigger_checked')->isVisible());
    ...
    +    $this->assertFalse($page->findField('textfield_in_details')->isVisible());
    ...
    +    $this->assertFalse($page->findField('textfield_invisible_when_checkbox_trigger_checked')->isVisible());
    ...
    +    $this->assertTrue($page->findField('textfield_in_details')->isVisible());
    

    This is exactly the stuff we used to do in WTB. What is the use of introducing BTB if we are not going to use it properly? i.e The way user interact with browser. This imo is just a WTB adapting JTB. I'm sorry but we are not setting a good example for adaption of BTB or JTB for rest of the core or contrib.

when we'll have enough tests, we'll think about refactoring.

It means we are allowing ourselves to continue practicing old WTB testing habits.

Providing many assertions right now means they are going to be there forever

Yes, that is exactly the intention here so that we'll adapt the browser testing, the way it suppose to be done, with interacting actual browser.

dawehner’s picture

@jibran
Thank you for looking at the quite big changes done by @DuaelFr. I totally agree with you that we should discuss about adding a lot of assertions vs. not adding a lot of assertions. Let's do that here: #2755617: Add assertions for javascript testing

This form is understandable now but not intuitive imho.

Well, if you just enable the test module and look at it, its equally clear, from just looking at it. On the other hand, when you actually would have to debug this test, you would see a difference:

+
+    $form['student_type'] = [
+      '#type' => 'radios',
+      '#options' => [
+        'high_school' => $this->t('High School'),
+        'undergraduate' => $this->t('Undergraduate'),
+        'graduate' => $this->t('Graduate'),
+      ],
+      '#title' => $this->t('What type of student are you?'),
+    ];
+    $form['high_school'] = [
+      '#type' => 'fieldset',
+      '#title' => $this->t('High School Information'),
+      // This #states rule says that the "high school" fieldset should only
+      // be shown if the "student_type" form element is set to "High School".
+      '#states' => [
+        'visible' => [
+          ':input[name="student_type"]' => ['value' => 'high_school'],
+        ],
+      ],
+    ];

vs.

+    $form['radios_trigger'] = [
+      '#type' => 'radios',
+      '#title' => 'Radios trigger',
+      '#options' => [
+        'value1' => 'Value 1',
+        'value2' => 'Value 2',
+        'value3' => 'Value 3',
+      ],
+    ];
....
   $form['fieldset_visible_when_radios_trigger_has_given_value'] = [
+      '#type' => 'fieldset',
+      '#title' => 'Fieldset visible when radio trigger has given value',
+      '#states' => [
+        'visible' => [
+          ':input[name="radios_trigger"]' => ['value' => 'value2'],
+        ],
+      ],
+    ];

@jibran Do you understand where we are coming from?

jibran’s picture

It is a very good idea to create #2755617: Add assertions for javascript testing so thank you for that.

On the other hand, when you actually would have to debug this test, you would see a difference

It is just a personal preference. I can live with this change.

+    $this->_testCheckboxTriggeredElements($page);
+    $this->_testTextfieldTriggeredElements($page);
+    $this->_testRadiosTriggeredElements($page);
+    $this->_testCheckboxesTriggeredElements($page);
+    $this->_testSelectTriggeredElements($page);
+    $this->_testMultipleTriggeredElements($page);

But can we please remove this? It can be a big test but if we want to split it in different methods then let's make multiple test methods.

dawehner’s picture

But can we please remove this? It can be a big test but if we want to split it in different methods then let's make multiple test methods.

Well yeah its slower, and well, things share certainly a common setup. I don't care to be honest.

DuaelFr’s picture

I made real test cases so core commiters can choose what they prefer between this patch and #40

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This test is incredible well documented!

jibran’s picture

Status: Reviewed & tested by the community » Needs work

We want to check all the states.

The following states may be applied to an element:
- enabled
- disabled
- required
- optional
- visible
- invisible
- checked
- unchecked
- expanded
- collapsed
- relevant
- irrelevant
- valid
- invalid
- touched
- untouched
- readwrite
- readonly
jibran’s picture

Here are some doc improvements and added helper methods in JSWebAssert and JavascriptStatesTest

dawehner’s picture

We want to check all the states.

While I agree with you on the point on we should do it, there is no requirement to do it as part of one issue. You know, we can always iterate and enlarge the test coverage over time.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Status: Needs review » Needs work
Issue tags: +Needs reroll
Manuel Garcia’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
21.93 KB
489 bytes

Reroll of #48, just a simple conflict on JSWebAssert.

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
cilefen’s picture

Title: Add Javascript tests for Form API's #states » Add Javascript tests for Form APIs #states
cilefen’s picture

Title: Add Javascript tests for Form APIs #states » Add Javascript tests for Form API's #states

Oh

dagmar’s picture

Didn't apply anymore. Added the interdiff of the updated part.

dagmar’s picture

Ups. wrong patch extension.

dagmar’s picture

Added new tests for Required and Optional elements.
Abstracted the logic to find a node in the page. It was repeated in four methods.

dagmar’s picture

The last submitted patch, 58: add_javascript_tests-2702233-58.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dagmar’s picture

Is this patch waiting to have all the test cases? @dawehner suggested in #49 we could commit this and iterate the missing tests cases in a follow up.

edurenye’s picture

I think @dawehner and @dagmar are right, and more seeing that there are issues like #2700667: Notice: Undefined index: #type in drupal_process_states() waiting for this to add their own cases.

jibran’s picture

Yeah, sure. Let's create a meta and track the #47 states and all the issues mentioned in #64.

tedbow’s picture

Title: Add Javascript tests for Form API's #states » Add Javascript tests for Form API's #states: require, visible, invisible
Parent issue: #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary » #2892872: [META] Add Javascript tests for all Form API's #states

Create meta issue #2892872: [META] Add Javascript tests for all Form API's #states

Updated the title reflect the states covered.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php
@@ -46,6 +48,157 @@ function isAjaxing(instance) {
+  public function assertElementVisible($selector_type, $selector, ElementInterface $container = NULL) {
...
+  public function assertElementNotVisible($selector_type, $selector, ElementInterface $container = NULL) {
...
+  public function assertElementRequired($selector_type, $selector, ElementInterface $container = NULL) {
...
+  public function assertElementOptional($selector_type, $selector, ElementInterface $container = NULL) {

Should these methods be made protected for move somewhere else and remain public?

otherwise this RTBC and would be good to commit.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should 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.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.