421473 - Move all tests from /mozilla/layout/reftests/svg/bugs to /mozilla/layout/svg/crashtests/

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

The files in this directory are crashtests but were created before the existence of the crashtests directory structure. They should be moved and the /mozilla/layout/reftests/svg/bugs directory deleted.

Comment 1

[Jonathan Watt [:jwatt]]

Good idea. How about doing that in the mozilla2 repository where we get real copying with history using Mercurial?

Comment 2

[Robert Longson [:longsonr]]

There's not that much in there so I'd be in favour of just making new cvs copies and deleting the originals. There's not that much cvs history to lose and they're only tests.

Comment 3

[Jonathan Watt [:jwatt]]

Fair enough.

Comment 4

[Mike Lierman]

What about the stats?
People actually look at those you know (I do sometimes)

Comment 5

[Robert Longson [:longsonr]]

What stats do you mean? They are crashtests and still will be but in a different location.

Comment 6

[Jonathan Watt [:jwatt]]

Still want to move these and delete the directory Robert? r=me if you do.

Comment 7

[Felipe Corrêa da Silva Sanches]

Attachment: move tests

Comment 8

[Daniel Holbert [:dholbert]]

Felipe: as jwatt alluded to in comment 1, it would be best to use "hg mv" rather than raw file-copying to perform these moves.  This lets mercurial know that the old file & new file are really the same & preserves the history on those files.  (There's probably not much history, but there's no need to destroy it when it's easy to preserve.)

To get the move correctly reported in your patch, you'll need to make sure you have git-style diffs enabled, as described here:
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_customize_the_format_of_the_patches_Mercurial_creates.3f

After you've got that set up, you should be able to do "hg mv [oldfilename] [newfilename]", and then "hg diff" will simply report the change as:
  diff --git a/[oldfilename] b/[newfilename]
  rename from [oldfilename]
  rename to [newfilename]
(The 'patch' tool doesn't understand those sorts of diffs, but 'hg import' does -- it's basically 'patch' with a few extensions.)

If you have any questions, feel free to ping me on irc.mozilla.org #developers

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to comment #8)
> Felipe: as jwatt alluded to in comment 1, it would be best to use "hg mv"

Note that comment 2-3 seemed to go against comment 1, but my understanding is that those comments were hoping to get this done sooner, before we fully switched to Mercurial.  Now that we've switched to Mercurial, there's no reason we shouldn't use "hg mv" to rename the files & preserve their history, IMHO.

Comment 10

[Daniel Holbert [:dholbert]]

>+++ b/layout/svg/crashtests/crashtests.list
>+load bug314244.xul # crash test
> load 322185-1.svg
> load 322215-1.svg
> load 323704-1.svg
> load 326495-1.svg
> load 326974-1.svg
>+load bug327709.svg # assertion test
[etc]

Also, you should remove the " # crash test" & " # assertion test" annotations in the lines added to crashtests.list.   (All the other files there are crash tests and/or assertion tests, too, so the annotations are unnecessary.  The annotations were only there in the first place because the tests were originally in the reftest directory & they weren't really reftests.)

Comment 11

[Jonathan Watt [:jwatt]]

Felipe, I agree with Daniel. If you can add a new patch using 'hg mv' as he suggests above that would be great.

Comment 12

[Felipe Corrêa da Silva Sanches]

Attachment: move tests using hg mv

Comment 13

[Felipe Corrêa da Silva Sanches]

Attachment: move tests with hg mv and remove old directory

Comment 14

[Felipe Corrêa da Silva Sanches]

Attachment: move with hg mv, remove original directory and remove reference to it

Comment 15

[Robert Longson [:longsonr]]

I'm not sure this patch came out like you wanted it to.

Comment 16

[Felipe Corrêa da Silva Sanches]

I have looked at the patch again and it seems to be exactly what I intended it to be.

Comment 17

[Robert Longson [:longsonr]]

Most people know hg better than me so it's likely just my lack of understanding ;-)

Comment 18

[Felipe Corrêa da Silva Sanches]

I'm still learning. So let's see what the reviewer says about this patch. I believe that it is correct, but it is the first time I use "hg mv".

Comment 19

[Daniel Holbert [:dholbert]]

Looks correct to me, with two nits:
- I think you forgot to add 367368.xhtml to the manifest in the destination directory.
- As long as you're renaming these files, could you add a "-1" suffix to them (e.g. "314244-1.xul"), to match the prevailing naming style?

Other than that, looks good! Not marking r+ yet, only because this patch is just a file-renaming patch and I'm asking you to modify your file-renaming. :)

Comment 20

[Daniel Holbert [:dholbert]]

Robert: I'm guessing that in comment 15, you were looking at bugzilla's "diff" view of the patch.  That view is kinda broken for |hg mv| / |hg cp| commands -- it's better to just look at the raw patch file.

Comment 21

[Felipe Corrêa da Silva Sanches]

Attachment: same thing as previous patches, with the fixes suggested

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 472469 [details] [diff] [review]
same thing as previous patches, with the fixes suggested

Looks good, thanks!

Comment 23

[Daniel Holbert [:dholbert]]

This patch doesn't need approval, since it's a tests-only fix, so I'm marking this bug as checkin-needed.

Felipe, if you get a chance, it'd be nice if you could update the patch with author + commit message as described here:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
so that whoever ends up checking this in has a slightly easier time.  But if you don't get to it, no worries.

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: fix for landing

Here's the fix again, with a minor reftest.list-bitrot fix & with checkin comment & committer set.  This is ready for landing.

Comment 25

[Daniel Holbert [:dholbert]]

Attachment: fix for landing

same fix again, now with "a=tests-only", in case it matters.

I think ehsan is about to land this as a ridealong. Thanks ehsan!

Comment 26

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/d4f58333d752