Page MenuHomePhabricator

MediaWiki core and MobileFrontend break branches REL1_25 and fundraising/REL1_25 and REL1_26 and REL1_27 tests
Closed, ResolvedPublic

Description

Steps to reproduce

REL1_25

REL1_26

  • scroll to bottom where you see the tests failures.

and https://integration.wikimedia.org/ci/job/mediawiki-phpunit-hhvm-trusty/744/console

Actual results

REL1_25

  • tests are failing with

18:15:11 There were 2 errors:
18:15:11
18:15:11 1) ApiLoginTest::testApiLoginBadPass
18:15:11 Trying to destroy uninitialized session
18:15:11
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/GlobalFunctions.php:3461
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/User.php:3574
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/User.php:3556
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/tests/phpunit/includes/api/ApiLoginTest.php:26
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/tests/phpunit/MediaWikiTestCase.php:131
18:15:11
18:15:11 2) ApiLoginTest::testApiLoginGoodPass
18:15:11 Trying to destroy uninitialized session
18:15:11
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/GlobalFunctions.php:3461
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/User.php:3574
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/User.php:3556
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/tests/phpunit/includes/api/ApiLoginTest.php:71
18:15:11 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/tests/phpunit/MediaWikiTestCase.php:131

REL1_26

15:08:21 There were 2 errors:
15:08:21
15:08:21 1) ApiLoginTest::testApiLoginBadPass
15:08:21 Trying to destroy uninitialized session
15:08:21
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/GlobalFunctions.php:3473
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/User.php:3639
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/User.php:3621
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/tests/phpunit/includes/api/ApiLoginTest.php:26
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/tests/phpunit/MediaWikiTestCase.php:131
15:08:21
15:08:21 2) ApiLoginTest::testApiLoginGoodPass
15:08:21 Trying to destroy uninitialized session
15:08:21
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/GlobalFunctions.php:3473
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/User.php:3639
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/includes/User.php:3621
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/tests/phpunit/includes/api/ApiLoginTest.php:71
15:08:21 /home/jenkins/workspace/mediawiki-phpunit-hhvm-trusty/src/tests/phpunit/MediaWikiTestCase.php:131

Expected results

The tests should pass without failure.

I think this https://phabricator.wikimedia.org/rMWdfd1256837ef7b259880b6cd470ce17487ac9379 may have broken the tests but I'm not sure.

MobileFrontend is only broken in mw core REL1_27 and mw core and mobilefrontend are only broken in REL1_25 and REL1_26

I haven't confirmed REL1_25 breaks with mobilefrontend but presume so since REL1_26 has been failing with similar errors.

Event Timeline

Paladox created this task.May 21 2016, 12:31 PM
Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptMay 21 2016, 12:31 PM

Adding @Legoktm since he said file a bug and cc him.

This also includes fundraising/REL1_25 which is also broken.

Paladox renamed this task from MediaWiki core REL1_25 tests are broken to MediaWiki core REL1_25 and fundraising/REL1_25 and REL1_26 tests are broken.May 21 2016, 3:34 PM
Paladox updated the task description. (Show Details)
Paladox updated the task description. (Show Details)May 21 2016, 3:36 PM
Paladox triaged this task as High priority.

Setting as high because the tests are broken and we need the tests to verify we doint break anything when uploading a patch and merging.

Paladox updated the task description. (Show Details)May 21 2016, 3:43 PM

I have confirmed that https://phabricator.wikimedia.org/rMWdfd1256837ef7b259880b6cd470ce17487ac9379 is indeed the patch that broke the tests.

The patch is in REL1_26 but under a different commit name.

See https://gerrit.wikimedia.org/r/#/c/290002/

MobileFrontend is now failing.

The patch was authored by @Bawolff.

@Bawolff can I add you to task please and should we revert that patch.

Paladox removed a subscriber: Bawolff.EditedMay 21 2016, 3:59 PM

MobileFrontend is now failing REL1_26 and REL1_27

See https://integration.wikimedia.org/ci/job/mediawiki-extensions-hhvm/63444/consoleFull and https://integration.wikimedia.org/ci/job/mediawiki-extensions-hhvm/63442/consoleFull please.

15:43:10 1) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #0 (true, true, true, true, true, array('a', '502-13'), 1, true, false)
15:43:10 in desktop view the cookie vary header should never be set
15:43:10 Failed asserting that true matches expected false.
15:43:10
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:156
15:43:10
15:43:10 2) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #1 (true, false, true, false, false, array('a', '502-13'), 0, true, false)
15:43:10 in desktop view the cookie vary header should never be set
15:43:10 Failed asserting that true matches expected false.
15:43:10
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:156
15:43:10
15:43:10 3) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #2 (false, true, true, true, true, array('a', '502-13'), 0, true, true)
15:43:10 in desktop view the cookie vary header should never be set
15:43:10 Failed asserting that true matches expected false.
15:43:10
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:156
15:43:10
15:43:10 4) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #3 (false, false, true, false, false, array('a', '502-13'), 0, true, false)
15:43:10 in desktop view the cookie vary header should never be set
15:43:10 Failed asserting that true matches expected false.
15:43:10
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:156
15:43:10
15:43:10 5) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #4 (true, true, false, true, true, array(), 1, false, false)
15:43:10 in desktop view the cookie vary header should never be set
15:43:10 Failed asserting that true matches expected false.
15:43:10
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:156
15:43:10
15:43:10 6) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #5 (true, false, false, false, false, array(), 0, false, false)
15:43:10 in desktop view the cookie vary header should never be set
15:43:10 Failed asserting that true matches expected false.
15:43:10
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:156
15:43:10
15:43:10 7) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #6 (false, true, false, true, true, array(), 0, false, true)
15:43:10 in desktop view the cookie vary header should never be set
15:43:10 Failed asserting that true matches expected false.
15:43:10
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:156
15:43:10
15:43:10 8) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #7 (false, false, false, false, false, array(), 0, false, false)
15:43:10 in desktop view the cookie vary header should never be set
15:43:10 Failed asserting that true matches expected false.
15:43:10
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:156
15:43:10
15:43:10 9) MobileFrontendHooksTest::testOnBeforePageDisplay with data set #8 (false, false, false, false, true, array(), 0, false, false)
15:43:10 in desktop view the cookie vary header should never be set
15:43:10 Failed asserting that true matches expected false.
15:43:10
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/extensions/MobileFrontend/tests/phpunit/MobileFrontend.hooksTest.php:77
15:43:10 /mnt/jenkins-workspace/workspace/mediawiki-extensions-hhvm/src/tests/phpunit/MediaWikiTestCase.php:156

Paladox renamed this task from MediaWiki core REL1_25 and fundraising/REL1_25 and REL1_26 tests are broken to MediaWiki core and MobileFrontend break branches REL1_25 and fundraising/REL1_25 and REL1_26 and REL1_27 tests.
Paladox updated the task description. (Show Details)

Change 290150 had a related patch set uploaded (by Brian Wolff):
Suppress session_destroy() warnings for unit tests

https://gerrit.wikimedia.org/r/290150

Bawolff added a comment.EditedMay 22 2016, 11:48 PM

The MF test failures appear to be caused by d707a90262a8b8 (T125283)

So, I can't figure out the reasoning for what MF is actually testing (Its testing there's no cookie vary, but I don't understand why it assumes there would never be no cookie vary), but I think its a false positive. Can someone familiar with mobile look at this test and determine if this is legit breaking something, or if the test should be changed? (ping @Florian)

Change 290150 had a related patch set uploaded (by Brian Wolff):
Suppress session_destroy() warnings for unit tests

https://gerrit.wikimedia.org/r/290150

This patch is for the core ApiLoginTest failures only, not the MF failures

Change 290151 had a related patch set uploaded (by Brian Wolff):
Suppress session_destroy() warnings for unit tests

https://gerrit.wikimedia.org/r/290151

Change 290153 had a related patch set uploaded (by Brian Wolff):
Follow-up ceffd37e68 Fix parser test that broke

https://gerrit.wikimedia.org/r/290153

So, I can't figure out the reasoning for what MF is actually testing (Its testing there's no cookie vary, but I don't understand why it assumes there would never be no cookie vary), but I think its a false positive. Can someone familiar with mobile look at this test and determine if this is legit breaking something, or if the test should be changed? (ping @Florian)

I think you're asking about the failing MobileFrontendHooksTest::testOnBeforePageDisplay test, right? I'm not entirely sure, why we omit this header, I assume there's (or at least there was) a good readon doing this because of something cache-related, but I'm not sure. The commit message, unfortunately, isn't really helpful :/ It's totally possible, that the header isn't needed anymore at all.

For the rationale of the test: I simply wrote it, because it was the expected behaviour at this time, I haven't really thought about other extensions adding the Cookie Vary header, sorry :(

Maybe @Jdlrobson knows why we need this header (or maybe don't need it anymore), so we can remove the test for it.

For the other parts of the same test case:
It checks, if:

  • The Vary: User-Agent header isn't added to the response, if not explicitly configured (there's a Google recommendation to do that, if the mobile and desktop URLs are the same), so it's part of important for our caching infrastructure to check, that we don't accidently send this header
  • Makes sure a X-Analytics header is set correctly
  • A canonical link is set on the mobile html pointing to the desktop url
  • An alternate link is set on the desktop html pointing to the mobile url

Both are important for how search engines handle the same content (with a different html structure) on different urls (probably not as important as it sounds, as the wmf infrastructure blacklists mobile urls from indexing, as far as I know)

So, probably we should think about that to do with the one check done in this test and maybe disable this specific part only or removing it completely, so that we get all other checks back as fast as possible :]

Change 290280 had a related patch set uploaded (by Paladox):
Suppress session_destroy() warnings for unit tests

https://gerrit.wikimedia.org/r/290280

Change 290280 merged by Ejegg:
Suppress session_destroy() warnings for unit tests

https://gerrit.wikimedia.org/r/290280

Change 290281 had a related patch set uploaded (by Ejegg):
Follow-up ceffd37e68 Fix parser test that broke

https://gerrit.wikimedia.org/r/290281

Change 290281 merged by Ejegg:
Follow-up ceffd37e68 Fix parser test that broke

https://gerrit.wikimedia.org/r/290281

Tgr added a subscriber: Tgr.May 24 2016, 6:01 PM

The post-SessionManager (1.27 and master) MF stuff is T135866.

This comment was removed by Paladox.

Change 290150 merged by jenkins-bot:
Suppress session_destroy() warnings for unit tests

https://gerrit.wikimedia.org/r/290150

Change 290153 merged by Gergő Tisza:
Follow-up ceffd37e68 Fix parser test that broke

https://gerrit.wikimedia.org/r/290153

Change 290151 merged by jenkins-bot:
Suppress session_destroy() warnings for unit tests

https://gerrit.wikimedia.org/r/290151

@Tgr anything left to do here?

Tgr closed this task as Resolved.May 27 2016, 10:17 PM
Tgr claimed this task.

I don't think so.