Page MenuHomePhorge

Fix regression in Remarkup unit tests and harden
Changes PlannedPublic

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

Details

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
arcpatch-D25847_1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1655
Build 1655: arc lint + arc unit

Event Timeline

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

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

also tried to fix PhutilRemarkupEngineTestCase

This revision is now accepted and ready to land.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 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 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)

\o/

This revision is now accepted and ready to land.Mon, Dec 9, 11:21
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 inline comments.
src/infrastructure/env/PhabricatorEnv.php
14

↑ Legacy documentation error.

valerio.bozzolan added inline comments.
src/infrastructure/markup/remarkup/__tests__/remarkup/link-square.txt
15

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