Page MenuHomePhabricator

Expand Differential test coverage to include moves, shields, and more
ClosedPublic

Authored by epriestley on Mar 5 2015, 12:54 AM.
Tags
None
Referenced Files
F13140361: D11970.diff
Fri, May 3, 3:39 AM
Unknown Object (File)
Wed, May 1, 11:17 AM
Unknown Object (File)
Mon, Apr 29, 3:41 PM
Unknown Object (File)
Wed, Apr 24, 11:40 PM
Unknown Object (File)
Thu, Apr 11, 7:58 AM
Unknown Object (File)
Apr 3 2024, 8:54 AM
Unknown Object (File)
Mar 30 2024, 10:58 AM
Unknown Object (File)
Mar 28 2024, 9:07 PM
Subscribers
Tokens
"Mountain of Wealth" token, awarded by btrahan.

Details

Summary

See D11468 and D11465. Fixes T5163. Fixes T4105. This makes it practical to test shields, unshielding, moves, etc.

This fixes the issue in D11468, where line maps from whitespace-ignored hunks could have fewer lines than line maps from whitespace-respected hunks, causing a warning.

This encodes the behavior which D11465 changed, making it the canon behavior. Specifically, we do not show a shield. I think this is correct. It seems misleading to show "the contents of this file were not changed", because they were changed in both the sense that the file was completely removed, and also changed in the sense that the content itself was (or may have been) changed at the destination. Instead, we just show nothing.

Test Plan
  • Added test coverage.
  • Ran tests.
  • Used arc diff --raw --browse to verify that web behavior was consistent with CLI/test behavior.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Expand Differential test coverage to include moves, shields, and more.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: btrahan, joshuaspence.

I haven't had a good chance to look at this yet, but here are two comments that I have so far.

src/applications/differential/__tests__/DifferentialParseRenderTestCase.php
83

Maybe rename this to buildChangesetParsers.

109

Maybe phtize this while you're here.

epriestley edited edge metadata.
  • Rename method.
  • pht()
  • Actually fix the moved file issue.

Specifically, since "moved" files count as "deleted", we were skipping preprocessing on them. This caused them to have no rendered content. If we comment out the early bailout, we get a boring/useless diff (albeit, not a broken one) where the whole file is red.

Instead, just render them with an empty "shield" so you can't reveal the content, even if it exists.

btrahan edited edge metadata.

awesometown

src/applications/differential/__tests__/DifferentialParseRenderTestCase.php
95

I think you should rebuild the parsers each test rather than doing this.

This revision is now accepted and ready to land.Mar 5 2015, 9:13 PM
epriestley edited edge metadata.
  • Remove clone cheating in unit test.
This revision was automatically updated to reflect the committed changes.