Page MenuHomePhabricator

Thanks is broken again (Mobile Thanks needs qunit tests)
Closed, ResolvedPublic

Description

For the nth time thanks is broken AGAIN.

http://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/158952

Exception thrown by ext.thanks.mobilediff
load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=minerva&version=20150108T…:150 Error: Module not found: loggingSchemas/MobileWebClickTracking Error: Module not found: loggingSchemas/MobileWebClickTracking

The fix should have a basic browser test/unit test for Thanks to ensure this doesn't break yet again. This has had far too many regressions.

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: Web-Team-Backlog.
Jdlrobson moved this task to Incoming on the Web-Team-Backlog board.
Jdlrobson subscribed.

I haven't verified this fix yet but we should add some basic tests to the thanks extension before we close this. This breakage happens far too often.

Jdlrobson renamed this task from Thanks is broken again to Thanks is broken again (Mobile Thanks needs qunit tests).Jan 14 2015, 7:50 PM
Jdlrobson added a project: Thanks.
Jdlrobson set Security to None.

I haven't verified this fix yet but we should add some basic tests to the thanks extension before we close this. This breakage happens far too often.

A browser test or client-side QUnit integration test for Thanks would help somewhat.

However, I don't think MobileFrontend Gerrit changes would trigger that test to run (so the failure would not get detected until later), so it's not a substitute for grepping and considering dependent extensions when changing the MF API.

This is fixed on beta labs but let's add a test to the Thanks extension before calling this done.

gerritbot subscribed.

Change 184976 had a related patch set uploaded (by Kaldari):
Mobile: Conform to new LoggingSchema

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

Patch-For-Review

Change 185093 had a related patch set uploaded (by Jdlrobson):
Add tests for rendering of thanks button on mobile diff page

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

Patch-For-Review

Change 184976 merged by Kaldari:
Mobile: Conform to new LoggingSchema

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

@MattFlaschen thanks to @hashar Thanks tests should run in MobileFrontend now on every commit so this https://gerrit.wikimedia.org/r/185093 qunit test should suffice!

@MattFlaschen thanks to @hashar Thanks tests should run in MobileFrontend now on every commit so this https://gerrit.wikimedia.org/r/185093 qunit test should suffice!

@Jdlrobson @hashar Great to hear. I didn't know Jenkins could be configured like that.

The job @Jdlrobson mentions is a shared one between mobile extensions but it is not including Thanks yet and it is only for the PHPUnit tests (not the qunit ones).

What we run for now is mwext-Thanks-testextension-zend which is defined in integration/config.git with the dependencies:

- Thanks:
  dependencies: 'Echo,Flow,Mantle'

Will try to get it included in the shared mobile job, and create a shared qunit one.

So I crafted a patch ( https://gerrit.wikimedia.org/r/#/c/185132/ ) which let one run on an extension patch 'check experimental'. That injects that extension to the shared job mediawiki-extensions-hhvm which clones several mobile extensions.

I have used a dummy Thanks patch ( https://gerrit.wikimedia.org/r/#/c/55136/ ) and gave it a try. The job cloned the extensions + Thanks and ... SUCCESS!

I am adding Thanks to the shared job and making it to trigger the shared job.

Change 185133 had a related patch set uploaded (by Hashar):
Add Thanks to the shared mw job

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

Patch-For-Review

Change 185133 merged by jenkins-bot:
Add Thanks to the shared mw job

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

Change 185134 had a related patch set uploaded (by Hashar):
Stop mwext-Thanks-testextension-zend on old branches

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

Patch-For-Review

Change 185134 merged by jenkins-bot:
Stop mwext-Thanks-testextension-zend on old branches

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

So the last check of Thanks https://gerrit.wikimedia.org/r/#/c/55136/5/ is working:

mediawiki-extensions-hhvm SUCCESS in 26s
mediawiki-extensions-zend SUCCESS in 32s
mwext-Thanks-lint SUCCESS in 0s

That is only for PHPUnit tests and not for old branches (such as REL1_24). The qunit jobs do not support that mechanism yet.

Change 183724 had a related patch set uploaded (by Hashar):
Have Thanks depend on MobileFrontend

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

Patch-For-Review

Change 183724 merged by jenkins-bot:
Have Thanks depend on MobileFrontend

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

I think that is solved, would let you close this task though.

Adding a qunit job for thanks is T86866: Thanks needs Jenkins qunit job.

Thanks @hashar
Yup so once https://gerrit.wikimedia.org/r/#/c/185093/ is fixed than Thanks will have its first QUnit test. T86866 when done will help get that merged.

I am adding Thanks to the shared job and making it to trigger the shared job.

For best results, changes to MobileFrontend should also trigger a shared job that tests Thanks master + the MobileFrontend change together.

In T86687#984452, @Mattflaschen wrote:

For best results, changes to MobileFrontend should also trigger a shared job that tests Thanks master + the MobileFrontend change together.

That is exactly how the shared job works :-P

@MattFlaschen or @kaldari can you merge that qunit patch? I'm keen to start having coverage for this asap.

Change 185093 merged by jenkins-bot:
Add tests for rendering of thanks button on mobile diff page

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

T88207 is to have the qunit job added to the shared jobs mediawiki-extensions-{hhvm,zend} which are shared by multiple repositories, thus ensuring a change to another extension does not break Thanks.