Page MenuHomePhabricator

Given I am logged in step not generic enough
Closed, DeclinedPublic

Description

This step checks if pt-logout is visible to check the login was successful. This is incompatible with the mobile skin (and possibly other skins) and was causing some browser test issues in T91665

Can we make this more generic?

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: Quality-Assurance.
Jdlrobson subscribed.
zeljkofilipin set Security to None.

As far as I can see, the mediawiki_selenium gem is still waiting for logout_element after logging the user in:

login_steps.rb

Given(/^I am logged in(?: as (\w+))?$/) do |user|
  as_user(user) do |user, password|
    visit(LoginPage).login_with(user, password)
  end
end

login_page.rb

class LoginPage
  button(:login, id: 'wpLoginAttempt')
  li(:logout, id: 'pt-logout')
  text_field(:password, id: 'wpPassword1')
  text_field(:username, id: 'wpName1')
  
  def login_with(username, password, wait_for_logout_element = true)
    username_element.when_present.send_keys(username)
    password_element.when_present.send_keys(password)
    login_element.when_present.click
    logout_element.when_present(10) if wait_for_logout_element
  end
end

An easy workaround would be to change

def login_with(username, password, wait_for_logout_element = true)

to

def login_with(username, password, wait_for_logout_element = false)

@Jdlrobson, @dduvall: what do you think?

This doesn't really seem any more logical. The problem with mobile is our log out element is his in the left menu. Ideally we should have a more generic way of asserting the login was a success.

There are a couple ways we can tackle this.

One possible solution is just to remove the wait on the logout element. Arguably, Given implementation shouldn't be asserting anything and you can always add your own assertions locally if that's desirable.

Another possibility would be to reopen the LoginPage class within the test suite of the mobile repository and redefine the selector for the :logout element.

dduvall raised the priority of this task from Medium to Needs Triage.Jun 2 2015, 3:48 PM
dduvall moved this task from Waiting to Next on the Browser-Tests-Infrastructure board.
zeljkofilipin lowered the priority of this task from Medium to Low.

Unlikely to ever be resolved because of T139740: Port Selenium tests from Ruby to Node.js.