Follow-up to #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary

Problem/Motivation

ToolbarIntegrationTest is not readable.

Proposed resolution

Perform the page level operation rather then working with css selectors.

Remaining tasks

Review
Commit

User interface changes

None

API changes

None

CommentFileSizeAuthor
#14 modernized-2711963-14.patch4.83 KBjibran
#14 interdiff.txt1.41 KBjibran
#3 modernized-2711963-3.patch6.94 KBjibran
#3 interdiff.txt3.52 KBjibran
#2 ToolbarIntegrationTestNG.patch3.42 KBjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext for University of Technology, Sydney commented
FileSize
ToolbarIntegrationTestNG.patch3.42 KB
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext for University of Technology, Sydney commented
FileSize
interdiff.txt3.52 KB
modernized-2711963-3.patch6.94 KB

Let's remove the obsolete functions as well.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Totally agree that this is much nicer. On the other hand I'm not sure whether its okay to remove those API functions now, it is sort of a unnecessary break. Do you mind keeping them and add a @deprecated with an explanation what one should do instead?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

The changes itself look awesome!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext for University of Technology, Sydney commented

As per my understanding JTB is in experimental state and #2469713: Step 2: Create a JavaScriptTestBase using PhantomJs Driver/Binary was committed just 15 days ago. I don't think these functions are worth keeping around but if you still don't agree then let me know @deprecate to which version?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

@jibran
Well I kinda agree, but you know, I just try to avoid issues with the committers later. Sometimes it is just not worth the hussle.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext for University of Technology, Sydney commented

Let's RTBC it and assign it to @alexpott. I don't mind deprecating those at all.

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

These changes make sense to me and are an easier example to follow for writing Javascript Tests.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

This looks much simpler, much better. And to @dawehner's point at #2702233-25: [backport] Add JavaScript tests for Form API #states: required, visible, invisible, expanded, checked, unchecked, this removes Drupal-specific assertions in favor of just using Mink's.

alexpott’s picture

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

I really think we should deprecate them - yes JTB is experimental but removing them unnecessarily breaks tests and obviously these tests will be new - that's a guaranteed way to annoy someone.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

Let's remove them in 8.3.x.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Good point!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran at PreviousNext for University of Technology, Sydney commented
Status: Needs work » Reviewed & tested by the community
FileSize
interdiff.txt1.41 KB
modernized-2711963-14.patch4.83 KB

interdiff is against #2. RTBCing it because it is just a document change.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

Thank you @jibran!

alexpott’s picture

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

Committed 0964a8f and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 7aab1f5 on 8.2.x 
    Issue #2711963 by jibran: Modernized ToolbarIntegrationTest

  • alexpott committed 0964a8f on 8.1.x 
    Issue #2711963 by jibran: Modernized ToolbarIntegrationTest

(cherry...

Status: Fixed » Closed (fixed)

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