Page MenuHomePhorge
Differential D25847

Fix regression in Remarkup unit tests and harden
Changes PlannedPublic
Actions

Authored by valerio.bozzolan on Thu, Dec 5, 15:52.

Details

Reviewers
aklapper
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15974: Do not consider emails and custom protocols as internal links
T15973: Fix unit test PhutilRemarkupEngineTestCase
T15967: Fix unit test PhutilPygmentizeParserTestCase
Summary

Fix regressions in two unit tests, because of a CSS addition from here:

https://we.phorge.it/rP349f006904fabf1d4df31ff4840502af3ab379a7

https://we.phorge.it/D25118

Some unit tests were missing the new CSS class 'remarkup-link-ext'. Added.

Also, we do not consider anymore 'mailto:info@example.com' as an internal link.

We hardened a bit the unit tests to also include mobile numbers in links, and alien protocols.

Closes T15967
Closes T15973
Closes T15974

Test Plan

Run these unit tests, and they finally work again:

arc unit ./src/infrastructure/parser/__tests__/PhutilPygmentizeParserTestCase.php
arc unit ./src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php

Additionally, run this, that has new tests added:

arc unit ./src/infrastructure/parser/__tests__/PhutilURIHelperTestCase.php

Also run this, check that the class tree is already correct:

arc liberate

Have fun with the green lights.

Diff Detail

Repository
rP Phorge
Branch
PhutilPygmentizeParserTestCase-fix
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1642
Build 1642: arc lint + arc unit

Event Timeline

valerio.bozzolan created this revision.Thu, Dec 5, 15:52
Owners added a reviewer: O1: Blessed Committers.Thu, Dec 5, 15:52
Herald added subscribers: Cigaryno, Matthew, tobiaswiese. · View Herald TranscriptThu, Dec 5, 15:52
valerio.bozzolan requested review of this revision.Thu, Dec 5, 15:52
Harbormaster completed remote builds in B1642: Diff 2557.Thu, Dec 5, 15:52
aklapper accepted this revision.Thu, Dec 5, 17:38
aklapper subscribed.

I applied this patch locally on top of git master and output does not complain anymore about 'link-brackets.txt' (thus it's correct) but fails in link-edge-cases.txt now (thus it's likely not complete):

   FAIL  PhutilRemarkupEngineTestCase::testEngine
Assertion failed, expected values to be equal (at PhutilRemarkupEngineTestCase.php:66): Failed to markup HTML in file 'link-edge-cases.txt'.
Expected vs Actual Output Diff
--- Old Value
+++ New Value
@@ -1,11 +1,11 @@
-'<p><a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a></p>
+'<p><a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a></p>
 
-<p>(<a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p>
+<p>(<a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p>
 
-<p><a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a></p>
+<p><a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a></p>
 
-<p><a href="http://www.example.com/wiki/example_(disambiguation)" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/wiki/example_(disambiguation)</a></p>
+<p><a href="http://www.example.com/wiki/example_(disambiguation)" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/wiki/example_(disambiguation)</a></p>
 
-<p>(example <a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p>
+<p>(example <a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a>)</p>
 
-<p>Quick! <a href="http://www.example.com/" class="remarkup-link" target="_blank" rel="noreferrer">http://www.example.com/</a>!</p>'
+<p>Quick! <a href="http://www.example.com/" class="remarkup-link remarkup-link-ext" target="_blank" rel="noreferrer">http://www.example.com/</a>!</p>'
This revision is now accepted and ready to land.Thu, Dec 5, 17:38
valerio.bozzolan planned changes to this revision.Mon, Dec 9, 09:43

but fails in link-edge-cases.txt now (thus it's likely not complete):

You are right. Marking as revision not ready.

But this is becoming a rabbit hole. Seems the Env/Configs are not configured correctly in unit tests, and we really need them:

arc unit src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php 
   FAIL  PhutilRemarkupEngineTestCase::testEngine
EXCEPTION (Exception): Trying to read configuration "phabricator.production-uri" before configuration has been initialized.
#0 /home/boz/repos/phorge/src/infrastructure/env/PhabricatorEnv.php(423): PhabricatorEnv::getEnvConfig('...')
#1 /home/boz/repos/phorge/src/infrastructure/env/PhabricatorEnv.php(447): PhabricatorEnv::getProductionURI('...')
#2 /home/boz/repos/phorge/src/infrastructure/env/PhabricatorEnv.php(441): PhabricatorEnv::getSelfURIMap()
#3 /home/boz/repos/phorge/src/infrastructure/parser/PhutilURIHelper.php(43): PhabricatorEnv::isSelfURI('...')
#4 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(80): PhutilURIHelper->isSelf()
#5 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(186): PhutilRemarkupDocumentLinkRule->renderHyperlink('...', '...')
#6 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(133): PhutilRemarkupDocumentLinkRule->markupDocumentLink(Array)
#7 [internal function]: PhutilRemarkupDocumentLinkRule->markupAlternateLink(Array)
#8 /home/boz/repos/phorge/src/infrastructure/markup/markuprule/PhutilRemarkupDocumentLinkRule.php(17): preg_replace_callback('...', Array, '...')
#9 /home/boz/repos/phorge/src/infrastructure/markup/blockrule/PhutilRemarkupBlockRule.php(92): PhutilRemarkupDocumentLinkRule->apply('...')
#10 /home/boz/repos/phorge/src/infrastructure/markup/blockrule/PhutilRemarkupDefaultBlockRule.php(17): PhutilRemarkupBlockRule->applyRules('...')
#11 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/PhutilRemarkupEngine.php(283): PhutilRemarkupDefaultBlockRule->markupText('...', NULL)
#12 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/PhutilRemarkupEngine.php(140): PhutilRemarkupEngine->markupBlock(Array)
#13 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/PhutilRemarkupEngine.php(106): PhutilRemarkupEngine->preprocessText('...')
#14 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php(56): PhutilRemarkupEngine->markupText('...')
#15 /home/boz/repos/phorge/src/infrastructure/markup/remarkup/__tests__/PhutilRemarkupEngineTestCase.php(13): PhutilRemarkupEngineTestCase->markupText('...')
#16 /home/boz/repos/arcanist/src/unit/engine/phutil/PhutilTestCase.php(639): PhutilRemarkupEngineTestCase->testEngine()
#17 /home/boz/repos/arcanist/src/unit/engine/PhutilUnitTestEngine.php(69): PhutilTestCase->run()
#18 /home/boz/repos/arcanist/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php(148): PhutilUnitTestEngine->run()
#19 /home/boz/repos/arcanist/src/workflow/ArcanistUnitWorkflow.php(170): ArcanistConfigurationDrivenUnitTestEngine->run()
#20 /home/boz/repos/arcanist/scripts/arcanist.php(427): ArcanistUnitWorkflow->run()
#21 {main}

I mark as changes planned, but I really don't know what to do.

Let's talk in new task T15973: Fix unit test PhutilRemarkupEngineTestCase

valerio.bozzolan updated this revision to Diff 2571.Mon, Dec 9, 09:51

also tried to fix PhutilRemarkupEngineTestCase

This revision is now accepted and ready to land.Mon, Dec 9, 09:51
Harbormaster completed remote builds in B1652: Diff 2571.Mon, Dec 9, 09:51
valerio.bozzolan planned changes to this revision.Mon, Dec 9, 09:51
valerio.bozzolan retitled this revision from Fix unit test PhutilPygmentizeParserTestCase to Fix regression in Remarkup unit tests.
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan added a task: T15973: Fix unit test PhutilRemarkupEngineTestCase.
valerio.bozzolan retitled this revision from Fix regression in Remarkup unit tests to Fix regression in Remarkup unit tests and harden.Mon, Dec 9, 11:21
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan added a task: T15974: Do not consider emails and custom protocols as internal links.
valerio.bozzolan updated this revision to Diff 2573.Mon, Dec 9, 11:21
valerio.bozzolan retitled this revision from Fix regression in Remarkup unit tests and harden to Fix regression in Remarkup unit tests.
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan removed a task: T15974: Do not consider emails and custom protocols as internal links.

\o/

This revision is now accepted and ready to land.Mon, Dec 9, 11:21
Harbormaster completed remote builds in B1653: Diff 2573.Mon, Dec 9, 11:21
valerio.bozzolan updated this revision to Diff 2574.Mon, Dec 9, 11:36

harden

Harbormaster completed remote builds in B1654: Diff 2574.Mon, Dec 9, 11:36
valerio.bozzolan updated this revision to Diff 2575.Mon, Dec 9, 11:42

arc unit

Harbormaster completed remote builds in B1655: Diff 2575.Mon, Dec 9, 11:42
valerio.bozzolan retitled this revision from Fix regression in Remarkup unit tests to Fix regression in Remarkup unit tests and harden.Mon, Dec 9, 11:42
valerio.bozzolan edited the summary of this revision. (Show Details)
valerio.bozzolan edited the test plan for this revision. (Show Details)
valerio.bozzolan added a task: T15974: Do not consider emails and custom protocols as internal links.
valerio.bozzolan added inline comments.
src/infrastructure/env/PhabricatorEnv.php
14 ↗(On Diff #2573)

↑ Legacy documentation error.

valerio.bozzolan updated this revision to Diff 2576.Mon, Dec 9, 11:55

last change promise lol

Harbormaster completed remote builds in B1656: Diff 2576.Mon, Dec 9, 11:55
valerio.bozzolan planned changes to this revision.Mon, Dec 9, 13:34
valerio.bozzolan added inline comments.
src/infrastructure/markup/remarkup/__tests__/remarkup/link-square.txt
15 ↗(On Diff #2576)

This is considered internal. Seems strange. Better to double-review.

Revision Contents
Changeset List

PathSizePackages
src/
infrastructure/
markup/
remarkup/
__tests__/
remarkup/
2 linesO1: Blessed Committers

Diff 2557

View Options

src/infrastructure/markup/remarkup/__tests__/remarkup/link-brackets.txt

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0