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

CommentFileSizeAuthor
#25 2782663-25.patch1.38 KBjofitz
#22 2782663-22.patch1.22 KBjofitz
#22 interdiff-19-22.txt665 bytesjofitz
#19 2782663-19.patch1.22 KBjofitz
#11 2782663-11.patch1.38 KBjofitz
#11 interdiff-9-11.txt806 bytesjofitz
#9 browsertest-sylog.patch1.38 KBklausi
browsertest-sylog.patch1.38 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Looks perfect reasonable. The if is coming from the initial testing commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Postponed

Postponing on the outcome of any decision about big-bang patch - see #2782663: Convert web tests to browser tests for syslog module

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Postponed » Reviewed & tested by the community

The current plan is to commit all conversion issues opened before 2016-Sept-01, right? We need to get a base line of committed browser tests in core to discover further problems with them, see the plan in #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase).

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Postponed

The current plan is to commit all conversion issues opened before 2016-Sept-01, right?

No, that is not the current plan. I have no idea who agreed to that, but it wasn't me and I don't think it was a committer. Let's please continue with the approach @dawehner, @Mile23, and I worked out, and respect my decision that these would not be committed on a per-module basis.

Per-module is almost never appropriate issue scope. See https://www.drupal.org/core/scope.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Postponed » Closed (duplicate)

Actually, I already marked all these tiny issues as duplicates.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Closed (duplicate) » Needs review

@xjm: to get a base line of running browser tests on the testbot the current proposal is to commit a couple of conversions, see #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase). Could you comment there to make an alternative proposal?

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.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
FileSize
browsertest-sylog.patch1.38 KB

Patch still applies, uploading again to trigger testbot.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs review » Needs work

Looks good, just a coding standards nit.

+++ b/core/modules/syslog/tests/src/Functional/SyslogTest.php
@@ -31,10 +31,8 @@ function testSettings() {
+      $field = $this->xpath('//option[@value=:value]', array(':value' => LOG_LOCAL6)); // Should be one field.

I know there's a proposal for inline comments after code but, as far as I know, it's not yet approved. Let's move the comment on its own line.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
interdiff-9-11.txt806 bytes
2782663-11.patch1.38 KB

Moved the comment onto its own line.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs review » Reviewed & tested by the community

Great, thank you!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs review

Won't nearly identical changes like this be needed everywhere xpath assertions are used? Not really sure it makes sense to patch them one test at a time?

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

Commented on #2807237: PHPUnit initiative with the suggestion of scoping to xpath assertions instead of per test. This comment I made above does still apply:

Per-module is almost never appropriate issue scope. See https://www.drupal.org/core/scope.

:) Thanks!

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs review » Reviewed & tested by the community

@xjm, I don't see a way to this in a single patch. It's not only xpath() but also moving the the classes, namespaces, referring the, base class, etc. Removing the module scope, we end with a huge, unreviewable patch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2782663-11.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
CreditAttribution: claudiu.cristea as a volunteer and at Webikon commented
Status: Needs work » Reviewed & tested by the community

Failure?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2782663-11.patch, failed testing.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2782663-19.patch1.22 KB

Re-rolled.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs review » Reviewed & tested by the community

Re-setting to RTBC in expectation.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/syslog/src/Tests/SyslogTest.php
@@ -31,10 +31,9 @@ public function testSettings() {
+      $field = $this->xpath('//option[@value=:value]', array(':value' => LOG_LOCAL6));

This introduces long array syntax, but as of now we should only have short array syntax in core.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
interdiff-19-22.txt665 bytes
2782663-22.patch1.22 KB

Ooops! I won't jump back to RTBC this time. #Embarrassing

Array syntax corrected.

The last submitted patch, 19: 2782663-19.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 22: 2782663-22.patch, failed testing.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
2782663-25.patch1.38 KB

I forgot to move the file.

boaloysius’s picture

boaloysius CreditAttribution: boaloysius as a volunteer commented

@Jo Fitzgerald. Why is it that 'interdiff 2782663-22.patch 2782663-25.patch' giving me a blank file?

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented

@boaloysius The interdiff is blank because the only difference is the location of the file (that is also why I did not upload an interdiff).

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

alexpott’s picture

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

Committed and pushed bd6f8f6 to 8.4.x and ddc8c5b to 8.3.x. Thanks!

  • alexpott committed bd6f8f6 on 8.4.x 
    Issue #2782663 by Jo Fitzgerald, klausi: Convert web tests to browser...

  • alexpott committed ddc8c5b on 8.3.x 
    Issue #2782663 by Jo Fitzgerald, klausi: Convert web tests to browser...

Status: Fixed » Closed (fixed)

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