[go: up one dir, main page]
Page MenuHomePhabricator
Create Task
Maniphest T316115

Fix app.js unit test in mobileapps
Closed, ResolvedPublicBUG REPORT
Actions

Description

Steps to replicate the issue :

run `./node_modules/mocha/bin/mocha test/features/app/app.js` inside mobileapps.

What happens?:

The test is failing with error:

express app
       should not follow redirects:

AssertionError [ERR_ASSERTION]: '<span class="mw-page-title-namespace">User</span><span class="mw-page-title-separator">:</span><span class="mw-page-title-main">BSitzmann (WMF)/MCS/Test/redirect test2</span>' == 'User:BSitzmann (WMF)/MCS/Test/redirect test2'
+ expected - actual

-<span class="mw-page-title-namespace">User</span><span class="mw-page-title-separator">:</span><span class="mw-page-title-main">BSitzmann (WMF)/MCS/Test/redirect test2</span>
+User:BSitzmann (WMF)/MCS/Test/redirect test2

This happened because of an upstream change in the core here: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/821353

What should have happened instead?:
This test should pass

Details

SubjectRepoBranchLines +/-
Fix unit tests in mobileapps for displaytitle propertymediawiki/services/mobileappsmaster+12 -9
Customize query in gerrit

Related Objects

Event Timeline

vadim-kovalenko created this task.Aug 24 2022, 12:53 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 24 2022, 12:53 PM
vadim-kovalenko changed the task status from Open to In Progress.Aug 24 2022, 12:53 PM
vadim-kovalenko claimed this task.
vadim-kovalenko added a subscriber: cscott.EditedAug 24 2022, 1:29 PM

Run mobileapps, check http://localhost:8888/test.wikipedia.org/v1/page/mobile-sections/User:BSitzmann_(WMF)%2FMCS%2FTest%2Fredirect_test2
In response, displaytitle and normalized properties are not the same now. Is this expected or there should be introduced some normalization inside mobileapps? Here is the place where the assertion is flailing.
@cscott , @MSantos

Sidenote: in https://test.wikipedia.org/api/rest_v1/page/mobile-sections/User%3ABSitzmann_(WMF)%2FMCS%2FTest%2Fredirect_test2_target_%25 properties displaytitle and normalizedtitle are the same

vadim-kovalenko triaged this task as High priority.Aug 24 2022, 1:30 PM
Aklapper added a project: Mobile-Content-Service.Aug 24 2022, 4:02 PM
Restricted Application added a project: Product-Infrastructure-Team-Backlog-Deprecated. · View Herald TranscriptAug 24 2022, 4:02 PM
vadim-kovalenko added a subscriber: Tacsipacsi.Aug 25 2022, 11:51 AM
• JMcLeod_WMF edited projects, added Page Content Service; removed Mobile-Content-Service, Content-Transform-Team.Aug 25 2022, 2:05 PM
• JMcLeod_WMF moved this task from Needs Triage to Blocked on the Page Content Service board.
Tacsipacsi added a comment.Aug 25 2022, 6:59 PM

I think that the tests are right here, and the actual code needs to be fixed. I don’t know what uses this endpoint, but I can imagine that it’s been broken by the value suddenly being HTML instead of plain text.

vadim-kovalenko mentioned this in T306440: Add markup to article headings to distinguish between the namespace and the title.Aug 26 2022, 7:34 AM
gerritbot added a comment.Aug 26 2022, 2:41 PM

Change 826878 had a related patch set uploaded (by Vadim Kovalenko; author: Vadim Kovalenko):

[mediawiki/services/mobileapps@master] Fix app.js unit test in mobileapps

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

gerritbot added a project: Patch-For-Review.Aug 26 2022, 2:41 PM
matmarex subscribed.Aug 29 2022, 5:51 PM
In T316115#8186756, @Tacsipacsi wrote:

I think that the tests are right here, and the actual code needs to be fixed. I don’t know what uses this endpoint, but I can imagine that it’s been broken by the value suddenly being HTML instead of plain text.

I'm not familiar with the endpoint either, but having HTML in display title is expected and I hope the existing code already handles it. For example, the page https://en.wikipedia.org/wiki/Vitamin_B6 has <sub>6</sub> in the display title. (And it indeed displays a subscript 6 in the iOS app, I can't easily check others now.)

Can you confirm that the issue here is just an overly strict test, and the newly added markup isn't causing any real issues for users?

vadim-kovalenko reassigned this task from vadim-kovalenko to MSantos.Aug 31 2022, 12:07 PM
gerritbot added a comment.Aug 31 2022, 12:52 PM

Change 826878 merged by jenkins-bot:

[mediawiki/services/mobileapps@master] Fix unit tests in mobileapps for displaytitle property

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

Maintenance_bot removed a project: Patch-For-Review.Aug 31 2022, 1:30 PM
MSantos added a comment.Sep 3 2022, 10:01 AM
In T316115#8195333, @matmarex wrote:
In T316115#8186756, @Tacsipacsi wrote:

I think that the tests are right here, and the actual code needs to be fixed. I don’t know what uses this endpoint, but I can imagine that it’s been broken by the value suddenly being HTML instead of plain text.

I'm not familiar with the endpoint either, but having HTML in display title is expected and I hope the existing code already handles it. For example, the page https://en.wikipedia.org/wiki/Vitamin_B6 has <sub>6</sub> in the display title. (And it indeed displays a subscript 6 in the iOS app, I can't easily check others now.)

Can you confirm that the issue here is just an overly strict test, and the newly added markup isn't causing any real issues for users?

Confirmed, no users affected and behavior is consistent between desktop and mobile.

MSantos closed this task as Resolved.Sep 3 2022, 10:01 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits