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

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran’s picture

FileSize
3.52 KB
6.94 KB

Let's remove the obsolete functions as well.

dawehner’s picture

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

The changes itself look awesome!

jibran’s picture

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

@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

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

almaudoh’s picture

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

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

This looks much simpler, much better. And to @dawehner's point at #2702233-25: Add Javascript tests for Form API's #states: require, visible, invisible, this removes Drupal-specific assertions in favor of just using Mink's.

alexpott’s picture

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

Let's remove them in 8.3.x.

Wim Leers’s picture

Good point!

jibran’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.41 KB
4.83 KB

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

dawehner’s picture

Thank you @jibran!

alexpott’s picture

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.