Problem/Motivation

Port over drupalPostWithFormat from WTB

Proposed resolution

Remaining tasks

  • Review
  • Address feedback
  • Commit

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#3 2795041-3.patch3.41 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

dawehner’s picture

Assigned: Unassigned » dawehner

Working on this particular subissue

dawehner’s picture

Assigned: dawehner » Unassigned
Status: Active » Needs review
FileSize
3.41 KB

Here is one implementation using guzzle

dawehner’s picture

dawehner’s picture

Issue summary: View changes
klausi’s picture

Title: BTB: Add drupalPostWithFormat » BrowserTestBase: Add drupalPostWithFormat

Why do we need this on BrowserTestBase? A test would only use this to test AJAX behavior, which we should convert to JavascriptTestBase instead?

  1. +++ b/core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
    @@ -88,4 +90,8 @@ public function error() {
    +  public function post(Request $request) {
    

    doc block is missing

  2. +++ b/core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
    @@ -86,4 +86,9 @@ public function testError() {
    +  public function testDrupalPostWithFormat() {
    

    doc block is missing

  3. +++ b/core/tests/Drupal/Tests/BrowserTestBase.php
    @@ -649,6 +649,35 @@ protected function buildUrl($path, array $options = array()) {
    +    $client = \Drupal::httpClient();
    

    let's not use \Drupal in classes when we don't have to.

    Does this even use the correct HTTP client of the curent test, like the Simpletest cookie and stuff?

dawehner’s picture

Thank you for your review!

Why do we need this on BrowserTestBase? A test would only use this to test AJAX behavior, which we should convert to JavascriptTestBase instead?

That's a good question. All calls in core are indeed related with with JS in some form or another:

Method
    drupalPostWithFormat
Found usages  (13 usages found)
    Method call  (13 usages found)
        d8  (13 usages found)
            core/modules/block/src/Tests/Views  (1 usage found)
                DisplayBlockTest.php  (1 usage found)
                    DisplayBlockTest  (1 usage found)
                        testBlockContextualLinks  (1 usage found)
                            366$response = $this->drupalPostWithFormat('contextual/render', 'json', $post, array('query' => array('destination' => 'test-page')));
            core/modules/config_translation/src/Tests  (1 usage found)
                ConfigTranslationUiTest.php  (1 usage found)
                    ConfigTranslationUiTest  (1 usage found)
                        renderContextualLinks  (1 usage found)
                            1101return $this->drupalPostWithFormat('contextual/render', 'json', $post, array('query' => array('destination' => $current_path)));
            core/modules/contextual/src/Tests  (1 usage found)
                ContextualDynamicContextTest.php  (1 usage found)
                    ContextualDynamicContextTest  (1 usage found)
                        renderContextualLinks  (1 usage found)
                            185return $this->drupalPostWithFormat('contextual/render', 'json', $post, array('query' => array('destination' => $current_path)));
            core/modules/editor/src/Tests  (1 usage found)
                EditorSecurityTest.php  (1 usage found)
                    EditorSecurityTest  (1 usage found)
                        testSwitchingSecurity  (1 usage found)
                            407$response = $this->drupalPostWithFormat('editor/filter_xss/' . $format, 'json', $post);
            core/modules/quickedit/src/Tests  (8 usages found)
                QuickEditAutocompleteTermTest.php  (1 usage found)
                    QuickEditAutocompleteTermTest  (1 usage found)
                        testAutocompleteQuickEdit  (1 usage found)
                            182$response = $this->drupalPostWithFormat('quickedit/entity/node/' . $this->node->id(), 'json', $post);
                QuickEditLoadingTest.php  (7 usages found)
                    QuickEditLoadingTest  (7 usages found)
                        testTitleBaseField  (2 usages found)
                            331$response = $this->drupalPostWithFormat('quickedit/metadata', 'json', $post);
                            388$response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post);
                        testUserWithoutPermission  (2 usages found)
                            113$response = $this->drupalPostWithFormat(Url::fromRoute('quickedit.metadata'), 'json', $post);
                            140$response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post);
                        testUserWithPermission  (3 usages found)
                            173$response = $this->drupalPostWithFormat('quickedit/metadata', 'json', $post);
                            245$response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post);
                            299$response = $this->drupalPostWithFormat('quickedit/entity/' . 'node/1', 'json', $post);
            core/modules/views_ui/src/Tests  (1 usage found)
                DisplayTest.php  (1 usage found)
                    DisplayTest  (1 usage found)
                        testPageContextualLinks  (1 usage found)
                            208$response = $this->drupalPostWithFormat('contextual/render', 'json', $post, array('query' => array('destination' => 'test-display')));

I just fear for contrib tests they could leverage that also for other ?_format=foo tests. Do we care about them?

dawehner’s picture

I'm happy to skip this method, but this might result in more porting work of existing tests.

klausi’s picture

I think that is fine - testing javascript should really use JavascriptTestBase, so even if porting is harder I think it is totally worth it.

Should we close this in the meantime? We can reopen if we find out that we need this method for something other than javascript testing.

dawehner’s picture

I think that is fine - testing javascript should really use JavascriptTestBase, so even if porting is harder I think it is totally worth it.

Oh yeah I totally agree on that, and well, at least for core, this seems to be indeed the case.

I'm just wonder about drupalGetWithFormat as that one is certainly used way more often, like in \Drupal\rest\Tests\Views\StyleSerializerTest which is unrelated to JS.

Maybe we should put both together into a trait, and call it a legacy day? To be honest for those tests you want to use guzzle, as you are not using a browser, you care about the HTTP requests.

dawehner’s picture

dawehner’s picture

Status: Needs review » Needs work

This is certainly not in review as of now :)

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

dawehner’s picture

Note: We certainly should throw a trigger_error here.

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.

Anonymous’s picture

#7, #10:

Why do we need this on BrowserTestBase?

We can reopen if we find out that we need this method for something other than javascript testing.

What about security tests? Why should we limit ourselves to the JS-API only? Running security tests in a JS environment can add an additional number of js/browser validators, which may not be on the backend. Accordingly, the tests will give a false sense of security.

I think BTB should support no less diverse API than WTB for the convenience of working with requests in different formats.

The choice of conversion WTB -> BTB or JTB is not related to this issue.

alexpott’s picture

I think BTB should support no less diverse API than WTB for the convenience of working with requests in different formats.

Adding methods with no usages tends to be problematic also we want to maintain less test API not more.

dawehner’s picture

I agree. I think being more explicit than implicit is also often better. One less question of how things work.

alexpott’s picture

Status: Needs work » Closed (won't fix)

So since @dawehner and I are both testing maintainers going to close this issue. 1 less in the queue.