Page MenuHomePhabricator

MobileFrontend patches will not merge due to browser test failure
Closed, ResolvedPublic

Description

Scenario: Search with JavaScript disabled # features/no_javascript_site.feature:34
is failing which is stopping us from merging code.

https://integration.wikimedia.org/ci/job/mwext-mw-selenium/682/console
Please can someone investigate.
If it's passing locally see T107908 for a related issue in Gather - it's probably due to steps incompleting. https://bocoup.com/weblog/a-day-at-the-races/ is a good reason.

Event Timeline

Jdlrobson raised the priority of this task from to High.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task to To Do on the Reading-Web-Sprint-54-28-Days-Later board.
Jdlrobson added a subscriber: Jdlrobson.

@phuedx @rmoen @bmansurov would be great if you can have a stab at this. Otherwise I'll try and get a patch in before we kick off next sprint (@Jhernandez and i are in strategy meeting till end of day).

rmoen set Security to None.
rmoen moved this task from To Do to Doing on the Reading-Web-Sprint-54-28-Days-Later board.

@Jdlrobson @bmansurov I can't even reach the failing step locally. Will try some more after lunch.

I don't suspect you will be able to reproduce this locally.
The trick is to make the steps more explicit.
So given https://integration.wikimedia.org/ci/job/mwext-mw-selenium/682/console fails on `features/step_definitions/search_steps.rb:42
14:56:21 object is read only {:name=>"search", :index=>0, :tag_name=>"input or textarea", :type=>"(any text type)"} (Watir::Exception::ObjectReadOnlyException)`

I woul suggest adding an "and page has loaded" step beforehand.
You'll unfortunately have to test the patch against Jenkins rather than locally.

@dduvall since these issues are coming up a lot (I suspect due to the tests running super super fast in jenkins) is there any better way to debug this / reduce these problems?

rmoen moved this task from Doing to To Do on the Reading-Web-Sprint-54-28-Days-Later board.

@dduvall since these issues are coming up a lot (I suspect due to the tests running super super fast in jenkins) is there any better way to debug this / reduce these problems?

I don't have any general advice beyond what's in the article you mentioned—nice find by the way!

For this particular problem, I can reproduce it consistently against Beta Cluster. FWICT, the custom user agent is not causing the site to serve up the no-JS version. In the step below, however, the value of the placeholder input is being set but the element is marked as read-only.

When(/^I type into search box "(.+)"$/) do |search_term|
  on(ArticlePage) do |page|
    if page.search_box2_element.exists?
      # Type in JavaScript mode
      page.search_box2 = search_term
    else
      page.search_box_placeholder = search_term
    end
  end
end

And now that I'm looking at the step definition, I do actually have some general advice. :) Avoid conditionals in tests as much as possible. Conditionals are a sign that you're not setting up and/or asserting preconditions early on. In this particular case, I would maybe add a step in the Given clause that asserts that the site did in fact serve up the no-JS version.

@dduvall, my findings exactly. Even though I'm currently blocked on T111133. I was able to determine that I am seeing a JS version of the mobile site when running the nojs tests.

I think this is a regression.
I'm seeing JavaScript site for browser with user agent

'Opera/9.80 (J2ME/MIDP; Opera Mini/9.80 (S60; SymbOS; Opera Mobi/23.348; U; en) Presto/2.5.25 Version/10.54')

@ori @Krinkle https://gerrit.wikimedia.org/r/#/c/234678/ seems to be the cause. We seem to be loading JavaScript when we shouldn't be.

Firefox used to allow the setting of user agents- did we upgrade the Firefox driver we use for these test builds?

The FF browser factory still supports custom user agents and I verified that it's working correctly (sorry, should have mentioned that). I would include an assertion within the step definition that sets the no-JS user agent—maybe assert that body.client-nojs is present?

Guess we caught a pretty serious regression... :)

\o/

In this case such an assertion wouldn't have helped since the class was being added but code loaded.

Really we need to get browser tests running against every core commit...

This is technical task.