See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase).

Postponed on:

#2863267: Convert web tests of views
#2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions
#2907485: Add getAllOptions() to AssertLegacyTrait

Follow up issues (for out-of-scope tests):

CommentCacheTagsTest.php
Uses Entity base class that is not converted yet, added note for this on #2862508: Convert system functional tests to phpunit.

CommentPagerTest.php
JavascriptTestBase conversion is done here #2809467: Convert \Drupal\comment\Tests\CommentPagerTest to JavascriptTestBase and BrowserTestBase. I'm going to expand that issue to convert the rest to BrowserTestBase as well.

CommentNewIndicatorTest.php
Most likely needs to be converted to JavaScriptTestBase: #2874133: Convert \Drupal\comment\Tests\CommentNewIndicatorTest to JavascriptTestBase and/or BrowserTestBase.

/update/CommentAdminViewUpdateTest.php
/update/CommentUpdateTest.php
These are both covered by #2905627: Part-2: Convert UpdatePathTestBase to BrowserTestBase

All other Comment tests are covered by the patches from #52 onwards.

CommentFileSizeAuthor
#73 report.txt11.08 KBmpdonadio
#73 interdiff-68-73.txt1.6 KBmpdonadio
#73 2866513-73.patch43.85 KBmpdonadio
#69 interdiff-67-68.txt734 bytesmpdonadio
#69 2866513-68.patch45.45 KBmpdonadio
#67 2866513-67.patch45.75 KBmpdonadio
#63 2866513-63-interdif-59-63.txt2.97 KBjonathan1055
#63 2866513-63.patch45.74 KBjonathan1055
#59 2866513-59.patch45.35 KBjonathan1055
#52 2866513-52-interdif-49-52.txt4.47 KBjonathan1055
#52 2866513-52.comment_tests_plus_legacy_getalloptions.patch46.65 KBjonathan1055
#49 2866513-49.patch40.88 KBjonathan1055
#46 2866513-45-interdif-43-45.txt896 bytesjonathan1055
#46 2866513-45.patch54.24 KBjonathan1055
#43 2866513-41-interdif-41-43.txt6.84 KBjonathan1055
#43 2866513-43.patch54.28 KBjonathan1055
#41 2866513-41.patch54.27 KBjonathan1055
#38 2866513-38.patch55.94 KBjonathan1055
#33 2866513-33.patch56.4 KBJo Fitzgerald
#33 interdiff-31-33.txt3.54 KBJo Fitzgerald
#31 2866513-31.patch43.67 KBmichielnugter
#31 interdiff-23-31.txt9.11 KBmichielnugter
#25 2866513-25-with-extras.patch61.8 KBmichielnugter
#25 2866513-25.patch43.31 KBmichielnugter
#25 interdiff-23-25.txt8.69 KBmichielnugter
#23 2866513-23.patch39.7 KBmichielnugter
#23 2866513-23-with-extras.patch56.75 KBmichielnugter
#18 2866513-18.patch56.71 KBmichielnugter
#16 2866513-16.patch52.48 KBmichielnugter
#14 2866513-14.patch52.24 KBmichielnugter
#10 2866513-10.patch62.68 KBmichielnugter
#9 2866513-9.patch59.13 KBmichielnugter
#7 2866513-3.patch47.77 KBmichielnugter
#2 2866513-02.patch29.05 KBmpdonadio
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

Status: Active » Postponed
Related issues: +#2863267: Convert web tests of views
FileSize
29.05 KB

Postponed on #2863267: Convert web tests of views, but attached is a start.

mpdonadio’s picture

Title: Convert web tests to browser tests for comment module » [PP-1] Convert web tests to browser tests for comment module
michielnugter’s picture

Assigned: Unassigned » michielnugter

Making a more extensive pass for each test to see what I can get to work with minimal effort. Will post a patch soon with a detailed update.

michielnugter’s picture

Title: [PP-1] Convert web tests to browser tests for comment module » [PP-2] Convert web tests to browser tests for comment module
Issue summary: View changes
Status: Postponed » Needs review
Related issues: +#2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions

Fixed some tests but then hit a wall, AssertLegacyTrait is not fully compatible on assertFieldByName and assertNoFieldByName. The old methods searched for multiple fields and the versions in AssertLegacyTrait only on 1. Separate issue created: #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions.

Also re-moved all the tests for a cleaner patch, it applies nicer in phpStorm now :)

Setting to review to get an idea on what the testbot thinks of it, this is in no way ready for a review :)

Status: Needs review » Needs work

The last submitted patch, 2: 2866513-02.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
47.77 KB

Status: Needs review » Needs work

The last submitted patch, 7: 2866513-3.patch, failed testing.

michielnugter’s picture

Update, lots more tests are working and validated not-working. Included some unrelated files (files from the issue this one is postponed on).

List of files:
ViewTestBase.php
AssertLegacyTrait.php
BrowserTestBaseTest.php
TestForm.php

Fixing postComment will fix several tests, hope I can continue tomorrow.

michielnugter’s picture

Update. All tests now checked, got some more tests working. Found some incorrectly defined test, hope it will run on the test-bot now so we can see if it agrees with my list.

Next up: fix postComment to get more tests to the working column.

michielnugter’s picture

Status: Needs work » Needs review

The last submitted patch, 9: 2866513-9.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2866513-10.patch, failed testing.

michielnugter’s picture

Status: Needs work » Needs review
FileSize
52.24 KB

Status: Needs review » Needs work

The last submitted patch, 14: 2866513-14.patch, failed testing.

michielnugter’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
52.48 KB

Fixed incorrect Trait usage, fails should go down.

Testbot reported more succes than me locally. I somehow have trouble with the user authentication stuff and permission checks. Gonna have to figure out why that is some day..

Status: Needs review » Needs work

The last submitted patch, 16: 2866513-16.patch, failed testing.

michielnugter’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
56.71 KB

Right, found an interesting thing on ContentTranslationUITestBase. The base class was already in core after the big chunk conversion (#2770921: Feb 21st: Convert chunk of WTB to BTB by just moving classes, changing use statements adding traits) but was actually never used in core. It didn't completely work yet either, making the CommentTranslationUITest fail.

New patch has some more passes, hoping actually that the test-bot will return only 1 fail: CommentNewIndicatorTest. Didn't convert that one yet as I still have to figure out an alternative to curlExec(). I'm not sure yet on wether this really needs to be a browser test or something else. Some help on that one would be appreciated.

Status: Needs review » Needs work

The last submitted patch, 18: 2866513-18.patch, failed testing.

michielnugter’s picture

Issue summary: View changes

Right, more fails but still closer. RssTest now fixed and other fails on the other tests. Progress!

michielnugter’s picture

Issue summary: View changes

Analyzed the error reports some more, placed CommentPagerTest.php out of scope because it needs a JavaScript test.

michielnugter’s picture

Issue summary: View changes

Discussed with lendude, placing CommentNewIndicatorTest.php out of scope. It will most likely need a conversion to a Javascript test.

michielnugter’s picture

Issue summary: View changes
FileSize
56.75 KB
39.7 KB

CommentTranslationUITest now working with updated AssertLegacyTrait for checkboxes and other small fixes.

Only one left: CommentPreviewTest. Would like someone's opinion on this one. There is a $this->setRawContent($content); to test resubmitting a comment. Not sure yet on what to do with that one.. I'm tempted to place it out of scope because it might stall this issue too long. Will decide on that once the issues that this one is postponed on get committed.

Lendude’s picture

in general $this->setRawContent() means 'Turn me into a kernel test'.

michielnugter’s picture

CommentPreviewTest and CommentCSSTest now working.

Every test in the comment module that can be converted to BrowserTestBase has been converted. Clean and extra's patch attached. Please review only the clean patch.

Bigger changes:
- Moved generatePermutations to AssertHelperTrait, seemed like a good place, it's a helper function after all.

The last submitted patch, 25: 2866513-25.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 25: 2866513-25-with-extras.patch, failed testing.

michielnugter’s picture

Issue tags: +phpunit initiative
dawehner’s picture

Title: [PP-2] Convert web tests to browser tests for comment module » [PP-1] Convert web tests to browser tests for comment module

Yeah one less issue.

michielnugter’s picture

Title: [PP-1] Convert web tests to browser tests for comment module » Convert web tests to browser tests for comment module

And we're good to go! Well, after I fix the last conversion issue that is :) Working on that right now.

michielnugter’s picture

Last one should be fixed now. Updated the follow-ups with references or issues, that should be covered as well now.

dawehner’s picture

+++ b/core/modules/comment/tests/src/Functional/CommentPreviewTest.php
@@ -105,16 +110,13 @@
-    // Reset the content of the page to simulate the browser's back button, and
-    // re-submit the form.
-    $this->setRawContent($content);
-    $this->drupalPostForm(NULL, [], 'Save');
+    // Go back and re-submit the form.
+    $this->getSession()->getDriver()->back();

This is quite amazing

+++ b/core/modules/comment/tests/src/Functional/CommentAnonymousTest.php
@@ -43,7 +43,7 @@ public function testAnonymous() {
-    $preview = (string) $this->cssSelect('.preview')[0]->asXML();
+    $preview = (string) $this->getSession()->getPage()->find('css', '.preview')->getHtml();

Why can't we use the existing cssSelect method which is out there? It is not even marked as deprecated on BrowserTestBase

Jo Fitzgerald’s picture

Replace $this->getSession()->getPage()->find('css', ...) with $this->cssSelect(...).

Status: Needs review » Needs work

The last submitted patch, 33: 2866513-33.patch, failed testing.

michielnugter’s picture

Status: Needs work » Postponed

Thanks @Jo Fitzgerald

Patch now fails because the field assertions in #2868019: AssertLegacyTrait field assertions not compatible with Simpletest assertions have been reverted, postponing untill we figure out the way forward on that one.

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.

jonathan1055’s picture

Status: Postponed » Needs review
jonathan1055’s picture

Patch from #33 re-rolled. There are errors and test failures when I run the tests locally, but worth seeing what the testbots make of it.

Status: Needs review » Needs work

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

jonathan1055’s picture

Status: Needs work » Needs review

Well, at least the patch does get applied now.

18:36:58 PHP Fatal error: Trait method generatePermutations has not been applied, because there are collisions with other trait methods on Drupal\simpletest\TestBase in /var/www/html/core/modules/simpletest/src/TestBase.php on line 25

I saw this loaclly. The function generatePermutations() has been added to /core/modules/simpletest/src/AssertHelperTrait.php but not removed from /core/tests/Drupal/Tests/Traits/Core/GeneratePermutationsTrait.php

jonathan1055’s picture

I do not know the reason for duplicating generatePermutations() in /core/modules/simpletest/src/AssertHelperTrait.php. It is needed for CommentCSSTest.php, so in that class I have added

+use Drupal\Tests\Traits\Core\GeneratePermutationsTrait;

and

+  use GeneratePermutationsTrait;

and removed the addition in AssertHelperTrait.php.

This is the only change from #38 and the tests now run locally. There are some failures, but I wanted to get things done one at a time.

Status: Needs review » Needs work

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

jonathan1055’s picture

That's good because (a) the tests now run and we do not get the colision of Trait method generatePermutations(), and (b) the test failures are exactly the same as on my local testing.

I do not know if anything has changed in the implementation of $this->cssSelect() but it returns an array, so we need to add [0] to get the object from which to call the method ->find(). Maybe this was working in the patch in #31 and before, I don't know, but it does not seem to work now. Adding [0] solves this - see the interdiff.

The patch also fixes the coding standards errors that were introduced in the patch and reported in the test output (I have not fixed the coding standard errors which already exist in the files which have no other changes - that is out of scope for this issue)

jonathan1055’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 43: 2866513-43.patch, failed testing. View results

jonathan1055’s picture

Status: Needs work » Needs review
FileSize
54.24 KB
896 bytes

One final fix - the core button text seems to have changed from "delete comments" to just "delete" so the test needs to be adjusted. This was supposed to be in the last patch, but I missed it.

jonathan1055’s picture

All tests pass, plus no new coding standards violations introduced.

Just for info, I found where the "Delete comments" button got changed in core and why that caused us a problem. The patch is in #1986606-362: Convert the comments administration screen to a view comment #362 and the commit was made to 8.5.x and 8.4.x on 29 July.

/core/modules/comment/src/Form/ConfirmDeleteMultiple.php
   public function getConfirmText() {
-    return $this->t('Delete comments');
+    return $this->t('Delete');
   }

The comment tests were also fixed in line with this:

/core/modules/comment/src/Tests/CommentAdminTest.php
-    $this->drupalPostForm(NULL, $edit, t('Delete comments'));
+    $this->drupalPostForm(NULL, [], t('Delete'));

plus equivalent changes in /modules/comment/src/Tests/CommentNonNodeTest.php and /modules/comment/src/Tests/CommentTestBase.php

However, the patch from #31 of our issue above left the existing CommentTestBase.php in place, and created a new file /modules/comment/tests/src/Functional/CommentTestBase.php. Perfectly fine, except this was based on the original before the core commit of 29 July, hence still had the old button text.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Nice work @jonathan1055

Just for info, I found where the "Delete comments" button got changed in core and why that caused us a problem. The patch is in #1986606-362: Convert the comments administration screen to a view comment #362 and the commit was made to 8.5.x and 8.4.x on 29 July.

That's fine and it was because we started using action plugins in #1986606: Convert the comments administration screen to a view

Patch looks good to me. Minimal code changes and green yay!

jonathan1055’s picture

FileSize
40.88 KB

Here's a patch with the same eventual output, but using diff -M90% --find-copies to show that the two CommentTestBase files are copies of the original, highlighting the minor changes. Easier to read and understand (and smaller).

Lendude’s picture

Status: Reviewed & tested by the community » Needs work

Hate to pull this back from RTBC (I agree that what we have so far is ready), but the scope of this is now a little bigger then it was when this started rolling.

The Views tests and Update tests can be converted here, and if there is some reason they can't be, we need something in the IS to indicate why.

jonathan1055’s picture

Issue summary: View changes

The tests that still remain unconverted in /comment/src/Tests are:

  • three that are listed as out-of-scope and have follow up issues linked in the IS
  • two in /Update/ which are now covered by the patch in #2905627: Part-2: Convert UpdatePathTestBase to BrowserTestBase, so I have updated the IS with this
  • three in /Views/ which I think are the ones you are referring to, that can now be converted

OK I will work on the /Views/ tests and see what I can do.

jonathan1055’s picture

I have converted the final three Comment tests in /src/Tests/Views. The patch also (temporarily) adds the getAllOptions() method into AssertLegacyTrait just for testing on d.o. but this is actually covered by #2907485: Add getAllOptions() to AssertLegacyTrait. Thanks Lendude.

One thing which had me mystified was that the WizardTest was failing in my local testing, both before the conversion (via run-tests.sh) and after conversion (via phpUnit). The problem was in the line $this->drupalGet($node_comment->toUrl('edit-form')->toString()); which gets the comment edit form. The url is 'comment/1/edit'.

Method drupalGet() can accept for the first parameter a url object or a string. If it gets an object then the full url is calculated and the correct result is returned. If it gets a string, the expectation is that this will be the url starting from the drupal root directory, and then the 'http://localpath/path-to-drupal-root' is added in front. But using ->toString() on the url object, as was done in this test, adds the 'path-to-drupal-root', so that when drupalGet() also adds this we have two lots of path-to-drupal-root. Clearly this does not happen on d.o. testing because the tests pass, but I was getting failures with 404 page not found. The test site I am using is named 'drupal85dev' so the correct url is 'http://localpath/drupal85dev/comment/1/edit'. However, the test code as it stands was giving me 'http://localpath/drupal85dev/drupal85dev/comment/1/edit' which does not exist.

Removing the ->toString() means that a url object is passed to drupalGet() and this solved the problem. All tests pass locally, but it will be interesting to see if this also passes on d.o. testsbots.

jibran’s picture

Title: Convert web tests to browser tests for comment module » [PP-1] Convert web tests to browser tests for comment module

Looks good to me can be RTBC after #2907485: Add getAllOptions() to AssertLegacyTrait.

jonathan1055’s picture

Issue summary: View changes

Thanks jibran.

Regarding #52 I am still concerned as to why the existing WizardTest passed on d.o. testbots but not locally, as this caused me several hours of effort, which would be nice to avoid someone else also wasting. It would be helpful to know what the local testing set-up/config should have been such that the tests would have passed before my change. Could it be that the testbots run in a Drupal environment placed immediately at the localpath root, such that the additional directory name (which got added twice in my local testing) is empty, and thus would not cause a problem. It seems unlikely that this has not been discovered before now. Maybe I will have to raise an issue in the testing queue.

dawehner’s picture

@jonathan1055
I'm quite sure (99.5%) that the testbots install drupal in a folder, so these kind of problems are detected. Feel free to try out https://www.drupal.org/node/2487065 ... which allows you to run tests locally, but honestly this documentation is a bit rough :(

Lendude’s picture

Yeah pretty sure it's installed in a folder. I've seen (and written I fear) a fair number of patches where the basepath was hardcoded to '/' in some tests and they would pass locally and then fail on d.o

jonathan1055’s picture

Thanks @dawehner and @Lendude. My problem was that existing tests which were passing on d.o. failed in my local testing (before any changes) so I had to work out why. I don't want to de-rail this issue, so can you suggest a better place to discuss this problem? It's not really a DrupalCI testbot issue, is it?

jibran’s picture

Title: [PP-1] Convert web tests to browser tests for comment module » Convert web tests to browser tests for comment module
Issue tags: +Needs reroll
jonathan1055’s picture

I have chcked and the existing patch still applies, once the change to AssertLegacyTrait.php is entirely removed. So no code changes and no interdiff, just patch 52 without the last chunk.

Lendude’s picture

+++ b/core/modules/comment/src/Tests/CommentTestBase.php
@@ -2,6 +2,8 @@
+@trigger_error('\Drupal\comment\Tests\CommentTestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\comment\Functional\CommentTestBase', E_USER_DEPRECATED);

@@ -12,6 +14,9 @@
+ * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Tests\comment\Functional\CommentTestBase instead.

+++ b/core/modules/comment/src/Tests/Views/CommentTestBase.php
@@ -2,13 +2,17 @@
+@trigger_error('\Drupal\comment\Tests\Views\CommentTestBase is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, use \Drupal\Tests\comment\Functional\Views\CommentTestBase', E_USER_DEPRECATED);
...
+ * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Tests\comment\Functional\Views\CommentTestBase instead.

The deprecations require a Change record and a @see/See to that CR. Should be some examples for a CR floating around in the other conversion issues.

jonathan1055’s picture

OK, thanks. I have created a change record https://www.drupal.org/node/2908490 - I thought it would be OK to have one record for both of these, given they are in the same module?
I have also seen #2908391: Add a rule for the expected formulation of deprecations so will follow that standard. Couple of questions:

  1. Is it better to use @trigger_error(__NAMESPACE__ . 'CommentTestBase' ... instead of @trigger_error('\Drupal\comment\Tests\Views\CommentTestBase ...? I have seen both used recently
  2. Can the deprecated class use the newly-moved class instead of having the full sourcecode duplicated?, and if so is that preferred?
Lendude’s picture

I think one CR is fine, they are both deprecated for the same reason after all. The CR looks good to me.

1. I personally prefer the __NAMESPACE__ version because I find it easier to read and it very clearly points to the current file (no staring at two long namespaces and trying to see if they are the same), but I don't think there is a rule currently.
2. Since the classes can contain (or in the future have) WebTestBase or BrowserTestBase specific logic, I think copying the code is the only good way forward. That way we are free to make changes in the new class that depend on the underlying test engine without worrying that we break something in the old class.

jonathan1055’s picture

FileSize
45.74 KB
2.97 KB

1. Yes I prefer the __NAMESPACE__ version. Another benefit is that the code line is shorter. Which is helpful because it becomes longer again now that I have added 'See' and the CR url to the message (as per the deprecation standards)
2. Of course, yes it is better to give freedom to change the new without having to worry about the old.

The interdiff shows two other changes I have made:
3. In the deprecated src/Tests/Views/CommentTestBase.php I believe the statement "use Drupal\views\Tests\ViewTestBase;" should not be removed. if it is deleted you cannot test the deprecation triggers via phpUnit, because you get "Class 'Drupal\comment\Tests\Views\ViewTestBase' not found in comment/src/Tests/Views/CommentTestBase.php"
4. The one-line description of the class was "Tests the argument_comment_user_uid handler." which is clearly left over from a copy-pasting. I have changed it to "Provides setup and helper methods for comment views tests." to match the description in src/Tests/Views/CommentTestBase.php

Jo Fitzgerald’s picture

Issue tags: -Needs reroll

Remove "Needs reroll" tag

jibran’s picture

Status: Needs review » Reviewed & tested by the community

All the feedback addressed after #53 so it's RTBC.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review

Updating issue credits to include Lendude for mentoring/reviews

  1. +++ b/core/modules/comment/tests/src/Functional/CommentPreviewTest.php
    @@ -105,16 +110,13 @@ public function testCommentPreviewDuplicateSubmission() {
    -    // Reset the content of the page to simulate the browser's back button, and
    -    // re-submit the form.
    -    $this->setRawContent($content);
    -    $this->drupalPostForm(NULL, [], 'Save');
    +    // Go back and re-submit the form.
    +    $this->getSession()->getDriver()->back();
    

    <3 swoon

  2. +++ b/core/modules/comment/tests/src/Functional/Views/CommentAdminTest.php
    @@ -18,13 +18,13 @@
    -  protected function setUp() {
    -    parent::setUp();
    +  protected function setUp($import_test_views = TRUE) {
    +    parent::setUp($import_test_views);
    

    Have we mixed up our test base here? i.e. we're extending from the generic CommentTestBase but we're using the $import_test_views for the one in the views namespace?

mpdonadio’s picture

Assigned: michielnugter » Unassigned
FileSize
45.75 KB

#63 doesn't apply. Just needs a rebase.

$ git checkout 2e3db9155376617d78a844fdb40001afa809f63c
$ git apply --index 2866513-63.patch
$ git rebase origin/8.5.x
First, rewinding head to replay your work on top of it...
Applying: 2866513-63.patch
Using index info to reconstruct a base tree...
M core/modules/comment/src/Tests/CommentTestBase.php
M core/modules/comment/src/Tests/Views/DefaultViewRecentCommentsTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/comment/tests/src/Functional/Views/WizardTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/RowRssTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/NodeCommentsTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/FilterUserUIDTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/DefaultViewRecentCommentsTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/CommentRowTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/CommentRestExportTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/CommentOperationsTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/CommentFieldNameTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/CommentFieldFilterTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/CommentEditTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/CommentAdminTest.php
Auto-merging core/modules/comment/tests/src/Functional/Views/ArgumentUserUIDTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentUninstallTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentTypeTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentTranslationUITest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentTokenReplaceTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentTitleTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentThreadingTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentStatisticsTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentRssTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentPreviewTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentNonNodeTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentNodeChangesTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentNodeAccessTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentLinksTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentLinksAlterTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentLanguageTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentInterfaceTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentFieldsTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentCSSTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentBookTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentBlockTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentAnonymousTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentAdminTest.php
Auto-merging core/modules/comment/tests/src/Functional/CommentActionsTest.php
Auto-merging core/modules/comment/src/Tests/CommentTestBase.php

jibran’s picture

Status: Needs review » Needs work

Yeah, #66.2 is not needed.

mpdonadio’s picture

#66-2, yup. CommentAdminTest passes locally for me w/ this change.

This applies to 8.4.x, and is a test improvement, so I am assuming this is still good for that.

[Crosspost, so patch number mismatch].

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

larowlan’s picture

Updating credits to include @dawehner for a review that changed the shape of the patch.
Not crediting myself

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

This patch looks great!

<3 swoon

:D

  1. +++ b/core/modules/comment/tests/src/Functional/CommentBookTest.php
    @@ -1,10 +1,11 @@
    +use Drupal\comment\Tests\CommentTestTrait;
    
    +++ b/core/modules/comment/tests/src/Functional/CommentLanguageTest.php
    @@ -1,18 +1,19 @@
    +use Drupal\comment\Tests\CommentTestTrait;
    
    +++ b/core/modules/comment/tests/src/Functional/CommentNonNodeTest.php
    @@ -1,16 +1,17 @@
    +use Drupal\comment\Tests\CommentTestTrait;
    

    We're moving all functional comment tests out of Drupal\comment\Tests. Shouldn't the trait also be moved then?

    Then these new use statements would also not be necessary.

  2. +++ b/core/modules/content_translation/tests/src/Functional/ContentTranslationUITestBase.php
    @@ -227,10 +227,10 @@ protected function doTestTranslationOverview() {
    -        $this->assertEqual($elements[0]->getText(), $entity->getTranslation($langcode)->label(), new FormattableMarkup('Label correctly shown for %language translation.', ['%language' => $langcode]));
    +        $this->assertEqual($elements[0]->getHtml(), $entity->getTranslation($langcode)->label(), new FormattableMarkup('Label correctly shown for %language translation.', ['%language' => $langcode]));
             $edit_path = $entity->url('edit-form', ['language' => $language]);
             $elements = $this->xpath('//table//ul[@class="dropbutton"]/li/a[@href=:href]', [':href' => $edit_path]);
    -        $this->assertEqual($elements[0]->getText(), t('Edit'), new FormattableMarkup('Edit link correct for %language translation.', ['%language' => $langcode]));
    +        $this->assertEqual($elements[0]->getHtml(), t('Edit'), new FormattableMarkup('Edit link correct for %language translation.', ['%language' => $langcode]));
    

    These changes are not in comment module. I think they were added accidentally?

mpdonadio’s picture

Here fix for 72-2. I didn't see where this crept in.

Re 72-1, attached is the usage report. Ignore the fact that it says drupal-8.3.x (my PhpStorm project name is messed up). There are 80 usages, and some in WTB code. In scope to move now?

jonathan1055’s picture

Thanks mpdonadio for the reroll and for removing that change from content_translation

Regarding the CommentTestTrait with 80 usages, I would suggest we do that in a follow-up issue? This current issue has already suffered several does-not-apply-needs-reroll and adding that big list of extra moves will make it unworkable. Let's get this in first, as a lot of work has been done and is virtually ready to be RTBC.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

#73: hah, good point — I totally missed those other usages! Definitely out of scope then.

Back to RTBC!

larowlan’s picture

Updating credits to include WimLeers for review

  • larowlan committed 73bba85 on 8.5.x
    Issue #2866513 by michielnugter, jonathan1055, mpdonadio, Jo Fitzgerald...

  • larowlan committed 0400901 on 8.4.x
    Issue #2866513 by michielnugter, jonathan1055, mpdonadio, Jo Fitzgerald...
larowlan’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed as 73bba85 and pushed to 8.5.x

Cherry-picked as 0400901 and pushed to 8.4.x to minimize conflicts going forward

Published change record.

Time to redo those stats for simpletest countdown - quite a few here - great work!

@jibran @dawehner probably have to update your drupalcon session slides too :-P

dawehner’s picture

Haha :)

michielnugter’s picture

Thanks for the commit @larowlan! This sure does help with the countdown :)

Status: Fixed » Closed (fixed)

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