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

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
SVG
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: longsonr, Assigned: Felipe Corrêa da Silva Sanches)

Tracking

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
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?
(Reporter)

Comment 2

10 years ago
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.

Comment 4

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

Comment 5

10 years ago
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.
(Assignee)

Comment 7

8 years ago
Created attachment 448550 [details] [diff] [review]
move tests
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:
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
(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
[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.)
Felipe, I agree with Daniel. If you can add a new patch using 'hg mv' as he suggests above that would be great.
(Assignee)

Comment 12

8 years ago
Created attachment 472353 [details] [diff] [review]
move tests using hg mv
Attachment #448550 - Attachment is obsolete: true
Attachment #472353 - Flags: review?
Attachment #448550 - Flags: review?(jwatt)
(Assignee)

Comment 13

8 years ago
Created attachment 472354 [details] [diff] [review]
move tests with hg mv and remove old directory
Attachment #472353 - Attachment is obsolete: true
Attachment #472354 - Flags: review?
Attachment #472353 - Flags: review?
(Assignee)

Comment 14

8 years ago
Created attachment 472355 [details] [diff] [review]
move with hg mv, remove original directory and remove reference to it
Attachment #472354 - Attachment is obsolete: true
Attachment #472355 - Flags: review?
Attachment #472354 - Flags: review?
(Reporter)

Updated

8 years ago
Attachment #472355 - Flags: review? → review?(dholbert)
(Reporter)

Updated

8 years ago
Assignee: nobody → felipe.sanches
(Reporter)

Comment 15

8 years ago
I'm not sure this patch came out like you wanted it to.
(Assignee)

Comment 16

8 years ago
I have looked at the patch again and it seems to be exactly what I intended it to be.
(Reporter)

Comment 17

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

Comment 18

8 years ago
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.
(Assignee)

Comment 21

8 years ago
Created attachment 472469 [details] [diff] [review]
same thing as previous patches, with the fixes suggested
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:
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.
Keywords: checkin-needed
Created attachment 473680 [details] [diff] [review]
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.
Created attachment 473682 [details] [diff] [review]
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!
Attachment #473680 - Attachment is obsolete: true

Comment 26

8 years ago
http://hg.mozilla.org/mozilla-central/rev/d4f58333d752
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED

Updated

8 years ago
Target Milestone: --- → mozilla2.0b6
You need to log in before you can comment on or make changes to this bug.