Problem/Motivation

See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase) and #2809161: Convert Javascript/AJAX testing to use JavascriptTestBase

Proposed resolution

Convert everything to BrowserTestBase. The javascript bit is just using the Field UI javascript, it's not testing anything related to comment module javascript, so lets just keep this simple.

  1. Convert testTwoPagers to JavaScriptTestBase
  2. Convert to BrowserTestBase

Postponed

Needs CommentTestBase for BrowserTestBase from #2866513: Convert web tests to browser tests for comment module

Remaining tasks

CommentFileSizeAuthor
#16 2809467-16.patch2.76 KBlendude
#7 2809467-7.patch20.39 KBmichielnugter
#7 interdiff-5-7.txt1.34 KBmichielnugter
#5 2809467-5.patch20.36 KBmichielnugter
#5 interdiff-3-5.txt11.3 KBmichielnugter
#3 2809467-3.patch19.77 KBmichielnugter

Comments

dawehner created an issue. See original summary.

michielnugter’s picture

michielnugter commented
Assigned: Unassigned » michielnugter
michielnugter’s picture

michielnugter commented
StatusFileSize
new 2809467-3.patch19.77 KB

Work in progress patch. I converted the test and the commentTestBase.

Fails on a JavaScript error, not sure yet whether it's an actual bug or an error in the test.

michielnugter’s picture

michielnugter commented
Status: Active » Needs work
michielnugter’s picture

michielnugter commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-3-5.txt11.3 KB
new 2809467-5.patch20.36 KB

I updated the test and it now passes. As far as I can see it's a complete conversion and therefore ready for a full review.

I partially converted the CommentTestBase to a functional javascript variant, only the required parts for this test were converted as they were the only parts that could be validated.

A direct convert lead to some problems though as the comment/reply/entity_type/field display's the form for both fields. I had to add a container selector to target the correct fields.

The test is not very heavy on the JavaScript testing, 'only' the form settings part is tested.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Postponed

Postponing until we figure out the random test fails:
#2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly
#2832672: [PP-1] Upgrade jcalderonzumba/* for better test performance

michielnugter’s picture

michielnugter commented
Status: Postponed » Needs review
StatusFileSize
new interdiff-5-7.txt1.34 KB
new 2809467-7.patch20.39 KB

Small update to use the new api where possible.

The mentioned random fails have been fixed, this issue is good for a proper review then?

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.

michielnugter’s picture

michielnugter commented
Title: Convert AJAX part of \Drupal\comment\Tests\CommentPagerTest to JavascriptTestBase » Convert \Drupal\comment\Tests\CommentPagerTest to JavascriptTestBase and BrowserTestBase
Issue summary: View changes
Related issues: +#2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase), +#2866513: Convert web tests to browser tests for comment module
michielnugter’s picture

michielnugter commented
Issue summary: View changes
Status: Needs review » Postponed

Self-review:

- Base class method 'setCommentSubject' is not used. Either add coverage or remove it.

michielnugter’s picture

michielnugter commented
Component: phpunit » comment.module
Issue tags: +phpunit initiative

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.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Postponed » Needs work

No longer postponed I'm thinking.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Title: Convert \Drupal\comment\Tests\CommentPagerTest to JavascriptTestBase and BrowserTestBase » Convert \Drupal\comment\Tests\CommentPagerTest to BrowserTestBase
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 2809467-16.patch2.76 KB

My proposed resolution: Convert everything to BrowserTestBase. The javascript bit is just using the Field UI javascript, it's not testing anything related to comment module javascript, so lets just keep this simple and use drupalPostForm.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This makes a ton of sense, I agree that this doesn't need to be a jstest, great work @Lendude!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 2650c43409 to 8.7.x and 2956937e08 to 8.6.x. Thanks!

  • alexpott committed 2650c43 on 8.7.x 
    Issue #2809467 by michielnugter, Lendude: Convert \Drupal\comment\Tests\...

  • alexpott committed 2956937 on 8.6.x 
    Issue #2809467 by michielnugter, Lendude: Convert \Drupal\comment\Tests\...

Status: Fixed » Closed (fixed)

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