Closed Bug 421473 Opened 12 years ago Closed 9 years ago

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


(Core :: SVG, defect)

Not set





(Reporter: longsonr, Assigned: juca)




(2 files, 5 obsolete files)

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.
Good idea. How about doing that in the mozilla2 repository where we get real copying with history using Mercurial?
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.
Fair enough.
What about the stats?
People actually look at those you know (I do sometimes)
What stats do you mean? They are crashtests and still will be but in a different location.
Still want to move these and delete the directory Robert? r=me if you do.
Attached patch move tests (obsolete) — Splinter Review
Attachment #448550 - Flags: review?(jwatt)
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:

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 #developers
(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.
>+++ 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

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.)
Felipe, I agree with Daniel. If you can add a new patch using 'hg mv' as he suggests above that would be great.
Attached patch move tests using hg mv (obsolete) — Splinter Review
Attachment #448550 - Attachment is obsolete: true
Attachment #472353 - Flags: review?
Attachment #448550 - Flags: review?(jwatt)
Attachment #472353 - Attachment is obsolete: true
Attachment #472354 - Flags: review?
Attachment #472353 - Flags: review?
Attachment #472354 - Attachment is obsolete: true
Attachment #472355 - Flags: review?
Attachment #472354 - Flags: review?
Attachment #472355 - Flags: review? → review?(dholbert)
Assignee: nobody → felipe.sanches
I'm not sure this patch came out like you wanted it to.
I have looked at the patch again and it seems to be exactly what I intended it to be.
Most people know hg better than me so it's likely just my lack of understanding ;-)
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".
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. :)
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.
Attachment #472355 - Attachment is obsolete: true
Attachment #472469 - Flags: review?(dholbert)
Attachment #472355 - Flags: review?(dholbert)
Comment on attachment 472469 [details] [diff] [review]
same thing as previous patches, with the fixes suggested

Looks good, thanks!
Attachment #472469 - Flags: review?(dholbert) → review+
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:
so that whoever ends up checking this in has a slightly easier time.  But if you don't get to it, no worries.
Keywords: checkin-needed
Attached patch fix for landing (obsolete) — Splinter Review
Here's the fix again, with a minor reftest.list-bitrot fix & with checkin comment & committer set.  This is ready for landing.
Attached patch fix for landingSplinter Review
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!
Attachment #473680 - Attachment is obsolete: true
Closed: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
You need to log in before you can comment on or make changes to this bug.