Page MenuHomePhabricator

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

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

Event Timeline

vadim-kovalenko changed the task status from Open to In Progress.Aug 24 2022, 12:53 PM
vadim-kovalenko claimed this task.

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

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.

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

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?

Change 826878 merged by jenkins-bot:

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

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

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.