Comments

naveenvalecha created an issue. See original summary.

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.

vaplas’s picture

Status: Active » Needs review
FileSize
7.48 KB

Let's do it!)

+++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationContextualLinksTest.php
@@ -118,60 +118,11 @@ public function testContentTranslationContextualLinks() {
-    // Check that the translate link appears on the node page.
...
-    $response = $this->renderContextualLinks(['node:node=1:'], 'node/' . $node->id());
-    $this->assertResponse(200);
-    $json = Json::decode($response);
-    $this->setRawContent($json['node:node=1:']);
-    $this->assertLinkByHref($translate_link, 0, 'The contextual link to translate the node is shown.');

+++ b/core/modules/content_translation/tests/src/FunctionalJavascript/ContentTranslationContextualLinksTest.php
@@ -0,0 +1,78 @@
+    // Check that the translate link appears on the node page.
+    $this->drupalLogin($this->translator);
+    $this->drupalGet('node/' . $node->id());
+    $translate_link = '/node/' . $node->id() . '/translations';
+    $links = $this->assertSession()->waitForElement('css', '[data-contextual-id^="node:node=1"] .contextual-links a[href^="' . $translate_link . '"]');
+    $this->assertNotNull($links);

Using a black box instead of parsing json response seems fair solution.

Status: Needs review » Needs work

The last submitted patch, 3: 2887813-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

vaplas’s picture

Status: Needs work » Needs review
FileSize
7.32 KB
2.97 KB
Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Another great conversion @vaplas!

  1. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationContextualLinksTest.php
    @@ -118,60 +117,11 @@ public function testContentTranslationContextualLinks() {
    -  protected function renderContextualLinks($ids, $current_path) {
    

    awesome that we can get rid of that!

  2. +++ b/core/modules/content_translation/tests/src/FunctionalJavascript/ContentTranslationContextualLinksTest.php
    @@ -0,0 +1,74 @@
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    

    This can just be {@inheritdoc}, but that can probably be fixed on commit.

vaplas’s picture

FileSize
7.29 KB
765 bytes

Thanks!
#6.2: Indeed! FunctionalJavascript/ContentTranslationContextualLinksTest.php is a new test and we don't need to copy the flaws from old test. Fixed.

xjm’s picture

Thanks @vaplas for fixing that. In general, I'd really like it if we not ask committers to fix things on commit. Providing a correct patch is always better, and committers might miss the comment, or it might mess up their workflows, and it bypasses the review process.

xjm’s picture

Title: Convert web tests to browser tests for content_translation module Part -2 » Convert web tests to browser tests for content_translation module Part 2

I think it's just part 2, not part -2 (what does that even mean?). ;) So fixing the title for the commit message. :)

  • xjm committed eef3306 on 8.5.x
    Issue #2887813 by vaplas: Convert web tests to browser tests for...
xjm’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed
rename from core/modules/content_translation/src/Tests/ContentTranslationContextualLinksTest.php
rename to core/modules/content_translation/tests/src/Functional/ContentTranslationContextualLinksTest.php

@@ -118,60 +117,11 @@ public function testContentTranslationContextualLinks() {
-  protected function renderContextualLinks($ids, $current_path) {

It's awesome to be able to get rid of this icky method.

I briefly asked myself whether this was too disruptive for a patch release. Generally we prefer to only break @internal APIs in minors, and tests that were subclassing ContentTranslationContextualLinksTest could have been relying on this method. I think that's highly unlikely, though, since it's not a base class. (Sometimes we deliberately subclass non-base classes to run their methods in child tests, but it's quite an edgecase.)

Since the disruption of not backporting these fixes is also fairly high (makes fixing actual bugs harder), I went ahead and backported it as well. So committed to 8.5.x and 8.4.x. Thanks!

  • xjm committed 2a5854e on 8.4.x
    Issue #2887813 by vaplas: Convert web tests to browser tests for...
vaplas’s picture

Thank you, @xjm! Absolutely!

During the removal of the method, I was not at ease with my heart, because:

  • it's someone's work that will disappear
  • it turns out not quite equivalent converting
  • child tests may be broken.

But the more I looked at this method, the greater the preponderance of the following arguments:

  • it is an explicit rudiment of simpletest js-testing problems
  • it tests the signs, not the real result (we do not need to know what format is returned, if only it worked)
  • it is obsolete, because it indicates that this is:

    Copied from \Drupal\contextual\Tests\ContextualDynamicContextTest::renderContextualLinks().

    but this is not so long ago (from 11.09.2013, after #2074037: Add drupalPostUrl() — drupalPost()/drupalPostAjax() are for forms only, D8 JS performs non-form HTTP requests).

  • if we keep it, we will also need to synchronize this test after changes in the algorithm of the rendering contextual linsk.
  • if someone relies on this method, he can easily return it, but even better, if he goes to a more correct test.

In other words, a simple js-test that verifies the final result (black-box testing) seems much more efficient.

Thanks for understanding again! 🎉

Lendude’s picture

@xjm thanks for committing! I'll bump it back to needs work next time, you are right of course that leaving stuff for the committers to fix is just the wrong mentality. It won't happen again! :)

Status: Fixed » Closed (fixed)

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