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
F13209955: D11970.id28821.diff
Fri, May 17, 3:02 AM
F13209954: D11970.id28852.diff
Fri, May 17, 3:02 AM
F13209953: D11970.id28856.diff
Fri, May 17, 3:02 AM
F13209952: D11970.id28820.diff
Fri, May 17, 3:02 AM
F13209951: D11970.id28849.diff
Fri, May 17, 3:02 AM
F13185967: D11970.diff
Sat, May 11, 3:20 AM
Unknown Object (File)
Tue, May 7, 6:04 AM
Unknown Object (File)
Fri, May 3, 3:39 AM
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
Branch
diff2
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4748
Build 4764: [Placeholder Plan] Wait for 30 Seconds

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
70 ↗(On Diff #28821)

Maybe rename this to buildChangesetParsers.

96 ↗(On Diff #28821)

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
60 ↗(On Diff #28852)

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.