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

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new ToolbarIntegrationTestNG.patch3.42 KB
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
StatusFileSize
new interdiff.txt3.52 KB
new modernized-2711963-3.patch6.94 KB

Let's remove the obsolete functions as well.

dawehner’s picture

dawehner
Primary language German
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
commented

The changes itself look awesome!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
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
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 Toronto, Canada
commented

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

almaudoh’s picture

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 🇧🇪🇪🇺
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 🇪🇺🌍
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 🇪🇺🌍
commented

Let's remove them in 8.3.x.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Good point!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new interdiff.txt1.41 KB
new 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
commented

Thank you @jibran!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
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.