Closed Bug 702662 Opened 13 years ago Closed 12 years ago

[diff] support file renames in the diff view

Categories

(Webtools Graveyard :: Elmo, defect)

Product:
Webtools Graveyard
Component:
Elmo
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: Pike, Assigned: Pike)

References

Details

Attachments

(2 files, 5 obsolete files)

add tests to remove files, with and without parser
51.19 KB, patch
peterbe
: review+
Details | Diff | Splinter Review
make the ui a bit more proof
396 bytes, patch
peterbe
: review+
Details | Diff | Splinter Review 
Claiming [diff] is an app while that waits to move...

Anywho: We have a few candidates right now where we're moving files around in the repos. It'd be nice if the diff view actually supported that, as in, showed "file renamed foo->bar, changed:" or something.

No idea how hard that is going to be. I recall that file moves only show up in the very changeset hg object that moved the file, so it might be tricky to find those in the multi-revision diff logic we have.
(In reply to Axel Hecht [:Pike] from comment #0)
> Claiming [diff] is an app while that waits to move...
> 
> Anywho: We have a few candidates right now where we're moving files around
> in the repos. It'd be nice if the diff view actually supported that, as in,
> showed "file renamed foo->bar, changed:" or something.
> 
Interesting. What are we showing in Elmo, the `hg diff`? Just like git diff, surely we can configure it to say renames rather than deletes and adds. 

> No idea how hard that is going to be. I recall that file moves only show up
> in the very changeset hg object that moved the file, so it might be tricky
> to find those in the multi-revision diff logic we have.

Is there an extra parameter we can add to make it a more user-friendly diff output?
The view in question is https://github.com/mozilla/elmo/blob/master/apps/shipping/views/__init__.py#L103, and it's not a plain diff, that's the point.

Looking at the mercurial code, patch.diff, there's copies.copies, which might get us there.
Note-to-self: Develop as a hotfix (not dev branch)
Been looking into this with little progress. (took me quite a while to get my master/slave-ball stuff up and running. 

So I did:

  $ hg rename file2.properties file22.properties
  $ hg commit -m "renamed from file2 -> file22"
  $ hg push

What I get in my hg serve is this: http://cl.ly/2Z2b2F1T343D2Q1b0Z0W
Thus the diff looks like this in Elmo http://cl.ly/2x0D3R141I2E1Z1Z1y0a
So I inspected how this analyzed by the mercurial lib inside our `diff` view and I added a print statement like this: http://cl.ly/3K1Y350o0o2j1R0Z0a3H and it spits this out: http://cl.ly/391W1e1f031l0e2U1L1a
 
I don't understand what you mean by "Looking at the mercurial code, patch.diff, there's copies.copies, which might get us there."
Are you saying there's a plugin or something we can use so that repo.status() gives us something we can work with?
From http://mercurial.selenic.com/wiki/MercurialApi
"repo.status() - returns a tuple of files modified, added, removed, deleted, unknown(?), ignored and clean in the current working directory "
:(

I see you can override the match class to be used (from mercurial.match.match) but that seems very wild to me. 

An alternative would of course be to manually try to figure out if it was a rename by comparing all added with all removed and see if the content was the same. However, what if the developer does::
 $ hg rename foo bar
 $ emacs bar
 $ hg add bar
 $ hg ci -m "renamed then edited a little bi"
Also, seems there's been some decent chunk of changes to the stock match function. 
https://developers.kilnhg.com/Repo/Kiln/largefiles/largefiles-mercurial/History/861c4cf25466
Alas, we're still on some old 1.5 version.
http://selenic.com/hg/file/41453d55b481/mercurial/copies.py#l87 is what I meant, which is called by diff in http://selenic.com/hg/file/41453d55b481/mercurial/patch.py#l1604.

I didn't look at what that actually returns, though.
So, I made two pushes. One which was just `hg rename file2.properties file22.properties` and one which was `hg rename file22.properties file33.properties; emacs file33.properties`

The code looks something like this:

    moved = copies(repo, ctx1, ctx2, repo[nullid])[0]
    print "moved"
    print moved
    match = None # maybe get something from l10n.ini and cmdutil
    changed, added, removed = repo.status(ctx1, ctx2, match=match)[:3]
    print "changed, added, removed"
    print (changed, added, removed) 

The output for the first one (just rename, no edit):

  moved
  {'browser/locales/en-US/file22.properties': 'browser/locales/en-US/file2.properties'}
  changed, added, removed
  ([], ['browser/locales/en-US/file22.properties'], ['browser/locales/en-US/file2.properties'])


The output for the second one (one rename, one edit):

  moved
  {'browser/locales/en-US/file33.properties': 'browser/locales/en-US/file22.properties'}
  changed, added, removed
  ([], ['browser/locales/en-US/file33.properties'], ['browser/locales/en-US/file22.properties'])

In other words, the fact that file33.properties was also changed, does not appear as a clue. 

I think what we need to do is to compare the content before and after and if the content is the same we show it as 1 single line saying:

    file2.properties renamed to file22.properties

With no individual diff lines to appear below. 
If there are content changes, we can try to display it like Github does:
http://cl.ly/0Q2h0z472O3w221C2s03
which is to show the "added" part of it but annotate the filename with information about the renaming. 

Tomorrow I'm going to dig deeper into how to do the content comparisons.
Mainly a note-to-self: This is how Github displays a pure rename that doesn't also involved and edit: http://cl.ly/1A042m0M1v1Y3m2w0T2Y
Yay! I cracked it. At least from a superficial point of view in that it looks like it's working like we want it to.

Example 1
---------
* no new files or renaming
* one line is edited
* another new line is added
http://cl.ly/3f0L0i1a3h0M3J3M2d0y


Example 2
---------
* file22.properties is renamed to file33.properties
* file33.properties is also edited
* one new line is added in the edit
http://cl.ly/2A1d4615102u2n0I2P0Z


Example 3
----------
* file2.properties is renamed to file22.properties
* no other editing (ie. new content identical)
http://cl.ly/0b0x312p0y180d0l300c

What I'm going to do next is figure out a way to write unit tests to prove all of these things.
Sweet, looks great.
Can you also detect copies? I actually expect both moves and copies in our inbox. Sorry for the late call on that.
Attached patch Manages renames in the diff view (obsolete) — — Splinter Review 
Visually looks the same as 
https://bugzilla.mozilla.org/show_bug.cgi?id=702662#c10

The unit tests added do:

1) basic repo creation and a simple commit
2) a commit which is only a file rename
3) a commit which is a rename followed by an addition
4) a bunch of checks on calling the view with missing GET parameters

Also, I check.py checked most of the code I've added and I might have spilled ever so slightly outside the new code.
Attachment #579754 - Flags: review?(l10n)
To talk about copies, think about

echo foo > foo
hg add foo
hg commit -m"first"
hg cp foo bar
echo foo2 >> foo
echo foobar >> bar
hg commit -m"second"
Some nice progress on this. This is how it looks like on a commit with is only the `hg cp file.properties file-copy.properties`
http://cl.ly/3X0H1w0U2F2M3B3u3G3e

This is what it would look like if I do both a `hg cp file33.properties file33-copy.properties` *and* a bit of editing. (I removed a line and added one)
http://cl.ly/1q1C371s250Z2J3G292P

Going to wrap up test coverage for this and then upload a new patch.
Attached patch Now with support for `hg cp` too (obsolete) — — Splinter Review 
Also made some slight improvements to the structure.
Attachment #579754 - Attachment is obsolete: true
Attachment #579754 - Flags: review?(l10n)
Attachment #580245 - Flags: review?(l10n)
Comment on attachment 580245 [details] [diff] [review]
Now with support for `hg cp` too

Review of attachment 580245 [details] [diff] [review]:
-----------------------------------------------------------------

This is a good start, but I'll r- on a few:

- copies and renames only work for files which the parser reads?
-- this is sadly going to happen, the move ahead of us is going to relocate files like search plugins.

- I'm concerned about the tests.
-- We have one view, with two different test architectures in two different test files

How about cherry-picking the test patch on develop onto master, and unify the two? I know that you and I have rather different attitude to fixtures, but it really bothers me to have one view tested completely different and spread all over the place.

Also, why did you create a decorator instead of using setUp and tearDown?

PS: Here's why the test I wrote is as it is: disk IO is expensive, so I'd rather create the repo once, and then work with one repo and the db than trying to create and destroy the stage every time.

::: apps/shipping/views/__init__.py
@@ +159,5 @@
> +            if path in moved:
> +                data = ctx1.filectx(moved[path]).data()
> +                data = _universal_newlines(data)
> +                p.readContents(data)
> +                a_entities, a_map = p.parse()

This parsing step and ...

@@ +164,5 @@
> +            elif path in copied:
> +                data = ctx1.filectx(copied[path]).data()
> +                data = _universal_newlines(data)
> +                p.readContents(data)
> +                a_entities, a_map = p.parse()

... this one above are now not protected against inner fails.

@@ +190,5 @@
>              c_map = {}
> +            if path in moved.values():
> +                data = ctx1.filectx(path).data()
> +                data = _universal_newlines(data)
> +                continue

This probably wants a comment that this file is handled as part of the corresponding 'added', right?

Also, you don't need to load the data?
Attachment #580245 - Flags: review?(l10n) → review-
Hopefully I've now thought of everything. 
The diff_app() view now has 100% test coverage. 

What I did was that I cherry-picked your tests from develop but then I `git rm apps/life/test_repos.py` for various reasons:
* I genuinely think that generic fixtures is a bad idea. Talked to 3 other web devs and they agree that each test should have its fixtures or fixture loading. There was events inside your setUpModule that is not relevant to some tests.
* The tests don't belong in life/tests. They belong in shipping/tests
* Creating and destroying 16 different repos (because there are 16 tests) takes 1.2 seconds on my computer which fast enough to not need any IO optimization.
* from your tests, the only one I needed to complete my test suite class was the one where we try to do a comparison across two different repos that have moved on. 

All this git cherry-picking followed by git rm makes me wonder what will happen when we merge this hotfix. Perhaps it won't remove the apps/life/tests/test_repo.py

What might be cryptic is that there's one case of `p.readContents(data)` that isn't wrapped in a try:except. That's deliberate. It's, as far as I can see, impossible to be unable to read the copy since if the original was "corrupt" there won't be any data. Hopefully it's clear from one of the tests. 

Phew! This was all harder than I had hoped. Hopefully we can enjoy these new features soon. If you want to, we can change this to a feature branch and land it on dev first and dogfood it with some hg repo data that would exercise it. Let me know what you prefer.
Attachment #580245 - Attachment is obsolete: true
Attachment #581122 - Flags: review?(l10n)
Comment on attachment 581122 [details] [diff] [review]
diff view supporting hg- renames and copies

Review of attachment 581122 [details] [diff] [review]:
-----------------------------------------------------------------

There are a hand-ful of issues with the patch, thus r-.

::: apps/shipping/tests.py
@@ +76,5 @@
> +
> +def repository_base_wrapper(func):
> +    @functools.wraps(func)
> +    def wrapper(self, *args, **kwargs):
> +        old_repository_base = settings.REPOSITORY_BASE

This is going to break, see https://github.com/mozilla/elmo/commit/13f8f71880ced764569038c8fcf90fef412aefa6 for how I fixed that the first time around.

Also, I'm still wondering why this is such a complicated wrapper instead of setUp and tearDown.

@@ +120,4 @@
>  
>          return appver, milestone
>  
> +class ShippingDiffTestCase(ShippingTestCaseBase):

I found these tests hard to read. Notably, I don't know what's tested and what's not, and where.

The tests should have comments and good names. I don't really understand the naming scheme here, and I'm pretty sure that _diff_app or _base are not specifying those tests beyond the test case itself. I don't see the difference between _app and _base either. So one thing is to get away with those.

An example would be test_diff_app_new_file_change. The test adds one new file. I'm a bit lost on whether that test covers what it intends to, judging by the name.

Thus renaming would help, and a comment to make that more obvious.

Also, I found encoding-related tests in places where I didn't suspect them. Maybe my expectations are off, or regroup the tests?

::: apps/shipping/views/__init__.py
@@ +143,5 @@
>      paths = ([(f, 'changed') for f in changed]
>               + [(f, 'removed') for f in removed]
>               + [(f, 'added') for f in added])
>      diffs = DataTree(dict)
> +    _broken_paths = []

I didn't grok the _broken_paths logic at all. I'm tempted to say it isn't needed.

@@ +161,1 @@
>              continue

There's still no support to mv or cp files that don't touch the parser.

Interestingly enough, one could picture a case where the file moves/copies from a name that has a parser to one that doesn't, or has a different parser.

I guess we should use the parser, and the one of the target.

@@ +161,5 @@
>              continue
>          if action == 'added':
> +            if path in moved:
> +                if moved[path] in _broken_paths:
> +                    data = ''

Setting the data to be empty isn't guaranteed to parse successfully.

Not that I understand how to even end up here. If we did, it'd be by chance of the order of how files show up in the sequence.

But then again, I don't understand _broken_paths, so ...

@@ +166,5 @@
> +                else:
> +                    data = ctx1.filectx(moved[path]).data()
> +                    data = _universal_newlines(data)
> +                p.readContents(data)
> +                a_entities, a_map = p.parse()

I disagree that this can't fail.

@@ +218,5 @@
>                  p.readContents(data)
>                  c_entities, c_map = p.parse()
>              except:
> +                # consider doing something like:
> +                # logging.warn('Unable to parse %s', path, exc_info=True)

This comment doesn't really apply just here.

Maybe it's better to factor the cat/read/parse into a single method, raising a well known exception or just returning empties?

Then move the comment there, or move it to the first caller.

::: requirements/dev.txt
@@ +1,3 @@
>  coverage
>  mock
> +-e git+git://github.com/jbalogh/check.git#egg=check.py

I don't think that's intentional, and probably doesn't belong into this patch?
Attachment #581122 - Flags: review?(l10n) → review-
(In reply to Axel Hecht [:Pike] from comment #19)
> Comment on attachment 581122 [details] [diff] [review]
> diff view supporting hg- renames and copies
> 
> Review of attachment 581122 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are a hand-ful of issues with the patch, thus r-.
> 
> ::: apps/shipping/tests.py
> @@ +76,5 @@
> > +
> > +def repository_base_wrapper(func):
> > +    @functools.wraps(func)
> > +    def wrapper(self, *args, **kwargs):
> > +        old_repository_base = settings.REPOSITORY_BASE
> 
> This is going to break, see
> https://github.com/mozilla/elmo/commit/
> 13f8f71880ced764569038c8fcf90fef412aefa6 for how I fixed that the first time
> around.
> 
Fixed. 

> Also, I'm still wondering why this is such a complicated wrapper instead of
> setUp and tearDown.
>

You're right. When I started I thought only a couple of tests would need it in that class. But now, there is no need since every method in the testcase class needs it. I'm changing to a plain old setUp/tearDown pattern. 

 
> @@ +120,4 @@
> >  
> >          return appver, milestone
> >  
> > +class ShippingDiffTestCase(ShippingTestCaseBase):
> 
> I found these tests hard to read. Notably, I don't know what's tested and
> what's not, and where.
> 
> The tests should have comments and good names. I don't really understand the
> naming scheme here, and I'm pretty sure that _diff_app or _base are not
> specifying those tests beyond the test case itself. I don't see the
> difference between _app and _base either. So one thing is to get away with
> those.
> 
> An example would be test_diff_app_new_file_change. The test adds one new
> file. I'm a bit lost on whether that test covers what it intends to, judging
> by the name.
> 
> Thus renaming would help, and a comment to make that more obvious.
> 
> Also, I found encoding-related tests in places where I didn't suspect them.
> Maybe my expectations are off, or regroup the tests?
> 
I've added some doc strings to all tests that better describe what they do. 
I'm sorry that there are so many different tests with similar names and only with small differences. However, the principles of testing is that it's better to do one test per problem and the order of tests should not matter.

I had to create this many to get that 100% test coverage.

> ::: apps/shipping/views/__init__.py
> @@ +143,5 @@
> >      paths = ([(f, 'changed') for f in changed]
> >               + [(f, 'removed') for f in removed]
> >               + [(f, 'added') for f in added])
> >      diffs = DataTree(dict)
> > +    _broken_paths = []
> 
> I didn't grok the _broken_paths logic at all. I'm tempted to say it isn't
> needed.
> 
Unfortunately it was a while ago since I wrote the code but from re-skimming the code it appears to be very much needed. 
It keeps track of which paths fails to be read. Then, if two paths need to be compared (because they have been copied or renamed) I need to know if one of them is broken. 

> @@ +161,1 @@
> >              continue
> 
> There's still no support to mv or cp files that don't touch the parser.
> 
> Interestingly enough, one could picture a case where the file moves/copies
> from a name that has a parser to one that doesn't, or has a different parser.
> 
> I guess we should use the parser, and the one of the target.

What? 
"mv or cp files that don't touch the parser" What do you mean? I'm lost. 

> 
> @@ +161,5 @@
> >              continue
> >          if action == 'added':
> > +            if path in moved:
> > +                if moved[path] in _broken_paths:
> > +                    data = ''
> 
> Setting the data to be empty isn't guaranteed to parse successfully.
> 
Ok. I changed that to this::

                if moved[path] in _broken_paths:
                    a_entities, a_map = [], {}
                else:
                    data = ctx1.filectx(moved[path]).data()
                    data = _universal_newlines(data)
                    p.readContents(data)
                    a_entities, a_map = p.parse()

> Not that I understand how to even end up here. If we did, it'd be by chance
> of the order of how files show up in the sequence.
> 
> But then again, I don't understand _broken_paths, so ...
> 
> @@ +166,5 @@
> > +                else:
> > +                    data = ctx1.filectx(moved[path]).data()
> > +                    data = _universal_newlines(data)
> > +                p.readContents(data)
> > +                a_entities, a_map = p.parse()
> 
> I disagree that this can't fail.
> 
Sigh. Again, the problem with me having forgotten half of this. 
I *think* the reasoning is that that `readContents(data)` call happens of a file that has already been read once before. And when it was previously read, if it failed it would end up in _broken_paths and this is checked for. It's basically like this:

try:
  read(path)
except:
  _broken_paths.append(path)
...
if path not in _broken_paths:
  read(path)


> @@ +218,5 @@
> >                  p.readContents(data)
> >                  c_entities, c_map = p.parse()
> >              except:
> > +                # consider doing something like:
> > +                # logging.warn('Unable to parse %s', path, exc_info=True)
> 
> This comment doesn't really apply just here.
> 
> Maybe it's better to factor the cat/read/parse into a single method, raising
> a well known exception or just returning empties?
> 
> Then move the comment there, or move it to the first caller.
> 
I hate empty `except:` blocks and because I didn't write it I don't know for sure what the possible exceptions might be. If I did (e.g. ReadParseError) I could wrap the whole loop in a try/except. For now, since the exceptions aren't know there's not a lot I can do. 

> ::: requirements/dev.txt
> @@ +1,3 @@
> >  coverage
> >  mock
> > +-e git+git://github.com/jbalogh/check.git#egg=check.py
> 
> I don't think that's intentional, and probably doesn't belong into this
> patch?

Yes. It's unrelated. I realised this mistake too late but realised it's pretty useful and after all, I needed check.py to deliver this code.
Addresses what I said in 
https://bugzilla.mozilla.org/show_bug.cgi?id=702662#c20
I.e. some points not changed due to lack of understanding.
Attachment #581122 - Attachment is obsolete: true
Attachment #585925 - Flags: review?(l10n)
(In reply to Peter Bengtsson [:peterbe] from comment #20)
> (In reply to Axel Hecht [:Pike] from comment #19)
> > @@ +161,1 @@
> > >              continue
> > 
> > There's still no support to mv or cp files that don't touch the parser.
> > 
> > Interestingly enough, one could picture a case where the file moves/copies
> > from a name that has a parser to one that doesn't, or has a different parser.
> > 
> > I guess we should use the parser, and the one of the target.
> 
> What? 
> "mv or cp files that don't touch the parser" What do you mean? I'm lost. 

Sneaking the answer to this up front:

if one does
hg mv foo/list.txt bar/list.txt
the code only ever stays within this block:

     for path, action in paths:
         lines = []
         try:
             p = getParser(path)
         except UserWarning:
-            diffs[path].update({'path': path,
-                                'isFile': True,
-                                'rev': ((action == 'removed') and request.GET['from']
-                                        or request.GET['to']),
-                                'class': action})
+            _broken_paths.append(path)
+            diffs[path].update({
+              'path': path,
+              'isFile': True,
+              'rev': ((action == 'removed') and request.GET['from']
+                     or request.GET['to']),
+              'class': action
+            })
             continue

And thus never get's the mv or cp notice.
Comment on attachment 585925 [details] [diff] [review]
fixes for last batch of feedback in review

Review of attachment 585925 [details] [diff] [review]:
-----------------------------------------------------------------

r- based on the previous review comments.

::: apps/shipping/tests.py
@@ +122,4 @@
>          return appver, milestone
>  
>  
> +class ShippingDiffTestCase(ShippingTestCaseBase):

This shouldn't inherit from ShippingTestCaseBase, and just be called DiffTestCase.

We're not using the mixin to test the assets at all, probably don't need that in this case, and we don't need the appver and milestone magic either.

Don't name the test class something with Shipping either, please, as this code should eventually just move to pushes, bug 562204.

@@ +137,5 @@
> +            shutil.rmtree(self._base)
> +        if self._old_repository_base is not None:
> +            settings.REPOSITORY_BASE = self._old_repository_base
> +
> +    def test_diff_app_file_change_addition(self):

The comments looks like a good improvement, but the test function can simply be test_file_change_addition. There's no app, and 'diff' is already in the class.

::: apps/shipping/views/__init__.py
@@ +192,1 @@
>          else:

I think we should have the 

if path in moved.values():
     continue

check here, and that should also get rid of _broken_paths.

That's making the code more consistent, i.e., we parse when we show a diff, and put the errors into the right cycle.
Attachment #585925 - Flags: review?(l10n) → review-
(In reply to Peter Bengtsson [:peterbe] from comment #20)
> (In reply to Axel Hecht [:Pike] from comment #19)
> 
> > @@ +218,5 @@
> > >                  p.readContents(data)
> > >                  c_entities, c_map = p.parse()
> > >              except:
> > > +                # consider doing something like:
> > > +                # logging.warn('Unable to parse %s', path, exc_info=True)
> > 
> > This comment doesn't really apply just here.
> > 
> > Maybe it's better to factor the cat/read/parse into a single method, raising
> > a well known exception or just returning empties?
> > 
> > Then move the comment there, or move it to the first caller.
> > 
> I hate empty `except:` blocks and because I didn't write it I don't know for
> sure what the possible exceptions might be. If I did (e.g. ReadParseError) I
> could wrap the whole loop in a try/except. For now, since the exceptions
> aren't know there's not a lot I can do. 

The parsers are not supposed to fail, encoding errors being the obvious exception from the tests. If a file triggers a bug in the parser, the whole view mustn't fall on it's face, though, and thus the library needs to be protected against the spanish inquisition.

That's not the point, though, the point is that the comment is at some random call where you lost it, and in no useful place to actually address it.

> > ::: requirements/dev.txt
> > @@ +1,3 @@
> > >  coverage
> > >  mock
> > > +-e git+git://github.com/jbalogh/check.git#egg=check.py
> > 
> > I don't think that's intentional, and probably doesn't belong into this
> > patch?
> 
> Yes. It's unrelated. I realised this mistake too late but realised it's
> pretty useful and after all, I needed check.py to deliver this code.

Sigh.
Blocks: 699122
I've got a follow-up patch on my repo now, https://github.com/Pike/elmo/compare/master...hotfix%2Fbug-702662-diff-mv-cp is the diff to master.

What's a good way to review that? Attach both the full and the incremental patch?
(In reply to Axel Hecht [:Pike] from comment #25)
> I've got a follow-up patch on my repo now,
> https://github.com/Pike/elmo/compare/master...hotfix%2Fbug-702662-diff-mv-cp
> is the diff to master.
> 
> What's a good way to review that? Attach both the full and the incremental
> patch?

Why can't you just upload a new attachment here and obsolete the previous one? That way I can look at the diff between this and the previous diff.
Sure. I'd probably want to land it as two changesets.

Follow up, test coverage now shows that there's no test for a removed file. The
c_entities, c_map = [], {}
line is red.
Attachment #585925 - Attachment is obsolete: true
Attachment #587201 - Flags: review?(peterbe)
We didn't have tests to remove files, so I'm adding one for parser and one for without.
Attachment #587201 - Attachment is obsolete: true
Attachment #587201 - Flags: review?(peterbe)
Attachment #587272 - Flags: review?(peterbe)
Comment on attachment 587272 [details] [diff] [review]
add tests to remove files, with and without parser

Review of attachment 587272 [details] [diff] [review]:
-----------------------------------------------------------------

r+ BUT one nit in the setUp() if you just fix the reference to the parent class instead of the grand parent I'm fine with this. 

Looking back, I think we actually built something really nice here. The test code looks really plain and neat and easy to read through.

::: apps/shipping/tests.py
@@ +102,5 @@
> +        def write(self, *msg):
> +            pass
> +
> +    def setUp(self):
> +        super(TestCase, self).setUp()

Typo. This should be `super(DiffTestCase, self).setUp()`.

Actually, I checked and my code had this typo originally but to the other super class. Seems we both made the mistake by fault for distracting you in the first place.
Attachment #587272 - Flags: review?(peterbe) → review+
Follow up, this triggered a test failure, as the hg on the cs is too new.

Simple patch, tested against both 2.0.2 (current) and 1.3.1, which is what's on bm-l10n-dashboard01.
Attachment #587364 - Flags: review?(peterbe)
Attachment #587364 - Flags: review?(peterbe) → review+
Whee, tests are green, and the moves/copies work. See https://l10n-stage-sj.mozilla.org/shipping/diff?to=a4181a4b100a&from=ff1402db9f06&tree=fx_aurora&repo=releases%2Fl10n%2Fmozilla-aurora%2For&url=&locale= for proof.

Some visual glitches on develop, but those are from the jquery.ui update, the twisties overlap with the files. I'll file a follow up on that.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee: nobody → l10n
Target Milestone: --- → 2.1
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: