Details
- Reviewers
joshuaspence btrahan - Group Reviewers
Blessed Reviewers - Maniphest Tasks
- T6857: Moved file is showing incorrectly in Differential
Diff Detail
- Repository
- rP Phabricator
- Branch
- master
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 3984 Build 4762: [Placeholder Plan] Wait for 30 Seconds Build 3997: [Placeholder Plan] Wait for 30 Seconds
Event Timeline
We need to start adding test coverage when we change diff rendering, this code is really complex/fragile and there are a lot of unintended effects and regressions, especially around these shields. Notably, this is fixing some kind of regression or fallout.
differential/__tests__/ has some ability to do rendering tests, via DifferentialChangesetOneUpTestRenderer and DifferentialChangesetTwoUpTestRenderer. I'm not sure if it supports shields yet -- but if not, we should expand the test renderers so they do. Basically, I think we're within striking distance of being able to get reasonable test coverage on this stuff, and it's fragile enough that we should do it before continuing to make rendering changes.
(If it's a ton of work I'm happy to do the legwork to make the tests writable, but my recollection is that the test stuff was in fairly good shape, just never got pushed the last mile.)
I just had a quick look at the code and must admit that I have no idea how this stuff works. I'll probably have a chance to figure it out in more detail over the weekend, but if you had some thoughts on how to go about testing this, that would be helpful too.
Sure, let me see if I can write some sort of similar test case and fill in whatever gaps we're missing.
Im finding the screenshots confusing since they aren't of the same thing, but apologies for however I broke this.
| src/applications/differential/parser/DifferentialChangesetParser.php | ||
|---|---|---|
| 609 | files can be moved and then changed in their new location. | |
| src/applications/differential/parser/DifferentialChangesetParser.php | ||
|---|---|---|
| 609 | Yes, but in this case we show the changes in the new location rather than the old location right? | |
| src/applications/differential/parser/DifferentialChangesetParser.php | ||
|---|---|---|
| 609 | I don't really know the implications of changing the $unchanged variable here, other than logically it could be false in this case. :/ | |
| src/applications/differential/parser/DifferentialChangesetParser.php | ||
|---|---|---|
| 609 | Neither... I'm happy to wait until we add some tests. | |
I rolled this into D11970 with coverage, although I think the "broken" behavior is better so I retained it (although maybe I'm not quite understanding). In either case, moves-with-changes and moves-without-changes seem to do reasonable things now, and have test coverage.