Closed Bug 785821 Opened 12 years ago Closed 11 years ago

layout/reftests/forms could do with some ordering

Categories

(Core :: Layout: Form Controls, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: lahabana, Assigned: reyre)

References

(Regressed 1 open bug)

Details

(Whiteboard: [good first bug][mentor=lahabana])

Attachments

(9 files, 9 obsolete files)

(deleted), patch
mounir
: review+
Details | Diff | Splinter Review
(deleted), patch
mounir
: review+
Details | Diff | Splinter Review
(deleted), patch
mounir
: review+
Details | Diff | Splinter Review
(deleted), patch
mounir
: review+
Details | Diff | Splinter Review
(deleted), patch
mounir
: review+
Details | Diff | Splinter Review
(deleted), patch
mounir
: review+
Details | Diff | Splinter Review
(deleted), patch
mounir
: review+
Details | Diff | Splinter Review
(deleted), patch
mounir
: review+
lahabana
: review+
Details | Diff | Splinter Review
(deleted), patch
mounir
: review+
lahabana
: review+
Details | Diff | Splinter Review
Currently this folder is a little bit of a mess. Some HTML tags have a folder and some don't. I think that for clearness purpose either all tags should have their own folder or none should have folders. I'm more for a folder for every tag. First it will reduce the size of layout/reftests/forms/reftest.list and second it will make anybody understand how the tests are organised.
Whiteboard: [good first bug]
Version: unspecified → Trunk
Whiteboard: [good first bug] → [good first bug][mentor=lahabana]
@lahabana - i am new here .. could you guide me how to get started on this bug?
(In reply to rb from comment #1) > @lahabana - i am new here .. could you guide me how to get started on this > bug? Hey, First welcome at Mozilla and thanks for getting involved in fixing bugs! This bug is more reordering than anything. Mozilla tests the layout thanks to reftests https://developer.mozilla.org/en-US/docs/Mozilla_automated_testing#reftest_(R) (They pretty much compare 2 pages that should be rendered identically) Currently the folder with the reftests of the forms is a real mess. Some tests are ordered by tag they test some not. The idea would be to order that with a one tag/one folder approach. To make the folder first more comprehensible and easier to browse. There also some (implicite) naming conventions: reftests shouldn't be prefixed with the tag name they test if they are in a folder with that name (a good example is the meter folder). The rest of the conventions are quite intuitive and you should get them quite easily. Also you'll have to change the reftest.list file in each folder and create it for new folders. Do you need help for the development process (hg, patches...)? at first this help: https://developer.mozilla.org/en-US/docs/Introduction I try to be quite available on the IRC #developers but I have a life so I'm not always here ;) #introduction is a really helpful place when you are beginning. Hope all that helps and welcome at Mozilla again!
Is this bug still applicable? If so I can do some work on it.
(In reply to Rick Eyre (:reyre) from comment #3) > Is this bug still applicable? If so I can do some work on it. Yes it is still applicable if you want to work on it would be great.
Assignee: nobody → rick.eyre
Do you want one huge patch? Or separate ones for each move and rename? Like one for moving textarea, one for textbox, etc? I've already gotten far along in the work before I thought that a smaller patch would be better, but I can go back and make them into smaller ones if you'd like.
Yes if you don't mind doing separate patches it would be perfect.
Attachment #762691 - Flags: review?(charly.molter)
Attachment #762692 - Flags: review?(charly.molter)
Attachment #762693 - Flags: review?(charly.molter)
Attachment #762694 - Flags: review?(charly.molter)
Attachment #762695 - Flags: review?(charly.molter)
Attachment #762696 - Flags: review?(charly.molter)
Attachment #762697 - Flags: review?(charly.molter)
Attachment #762698 - Flags: review?(charly.molter)
Attachment #762691 - Flags: review?(charly.molter) → review+
Attachment #762692 - Flags: review?(charly.molter) → review+
Attachment #762693 - Flags: review?(charly.molter) → review+
Attachment #762694 - Flags: review?(charly.molter) → review+
Attachment #762695 - Flags: review?(charly.molter) → review+
Attachment #762696 - Flags: review?(charly.molter) → review+
Attachment #762697 - Flags: review?(charly.molter) → review+
Attachment #762698 - Flags: review?(charly.molter) → review+
Everything looks great! Just pushed it to try: https://tbpl.mozilla.org/?tree=Try&rev=66486ea0873e Just a few things: - You should add a folder for input/text, input/percentage and input/hidden. - In the input folder and subfolders removing the prefixes in the previous tests. For example input/email/input-email-1.html should become input/email/1.html. If you could do these 2 extra patches it would be perfect. Thank you very much for your work!
No worries! It was my pleasure :). Learnt a lot about reftests doing this bug. Would you like me to just put those two patches on top of these ones or get rid of the patch for moving the "input-" tests and in it's place put the one moving the "input-" tests to the various folders?
Rick, could you use `hg mv` to move those tests? That way, it makes the patches easier to read but also - and most important -, using `hg mv` keeps the history of the moved file.
Hmm, I guess git records renames different than mercurial? I'll fix that up and try to get it back to you soon.
Sorry Rick I didn't know about `hg mv`... For the two extra patches as you are going to redo the patches you can include the modifications in the `input-` patches
No worries Charly :). It shouldn't take to long if there is a way to mass rename files while using 'hg mv'. I'll have to look into it.
Sorry this is taking me a while. I'm still working on it and will have a patch this week.
I've generated the patch in git with --find-renames and it shows the renames and seems to apply to mercurial repositories as if you did 'hg mv' on the files. Is this alright Mounir?
Attachment #762691 - Attachment is obsolete: true
Attachment #771642 - Flags: feedback?(mounir)
Comment on attachment 771642 [details] [diff] [review] Part 1 v2: Move textarea element's to their own folder Review of attachment 771642 [details] [diff] [review]: ----------------------------------------------------------------- This patch only shows the reftest changes. There is no file moved here.
Attachment #771642 - Flags: feedback?(mounir) → feedback-
For some reason Bugzilla doesn't pick up the diff renames as renames so it's not showing when you click on diff or review. If you click on details you can see them there. Mercurial picks up the renames just like it would if you 'hg mv' it. If you want me do a mercurial style patch I can do that instead.
Comment on attachment 771642 [details] [diff] [review] Part 1 v2: Move textarea element's to their own folder Indeed. I think the reason is because the style of your patch file is very unusual. It's not a mercurial patch. Usually, we attach mercurial patches with git format, see: https://developer.mozilla.org/en/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F If you can generate a patch like that that would be better because this is the kind of patch we expect.
Attachment #771642 - Flags: feedback- → feedback+
Rick, it seems that I can correctly import the patch in this format so feel free to attach patches in this format for this bug. I will import them and push them.
I can upload the patches in the format that is specified in the MDN article. I've done all the work in git so it's no problem for me to apply the patches to Mercurial and then generate another patch in Mercurial that will look like what is expected. Thanks for the help Mounir.
Changed diff format to show renames. Carrying forward r=lahabana
Attachment #771642 - Attachment is obsolete: true
Attachment #772078 - Flags: review?(mounir)
I've generated it with hg using the settings discussed in the link, but it still doesn't seem to be showing the renames in the 'review' or 'diff' views. They are there under the 'details' view though.
Attachment #772078 - Flags: review?(mounir) → review+
Rick, could you do the same thing for other patches?
Will do now Mounir -- was just waiting for your r+ on the first one :)
Carrying forward r=lahabana
Attachment #762692 - Attachment is obsolete: true
Attachment #772147 - Flags: review?(mounir)
Carrying forward r=lahabana
Attachment #762693 - Attachment is obsolete: true
Attachment #772150 - Flags: review?(mounir)
Carrying forward r=lahabana
Attachment #762694 - Attachment is obsolete: true
Attachment #772151 - Flags: review?(mounir)
Carrying forward r=lahabana
Attachment #762695 - Attachment is obsolete: true
Attachment #772152 - Flags: review?(mounir)
Carrying forward r=lahabana
Attachment #762696 - Attachment is obsolete: true
Attachment #772154 - Flags: review?(mounir)
Carrying forward r=lahabana
Attachment #762697 - Attachment is obsolete: true
Attachment #772155 - Flags: review?(mounir)
Attachment #762698 - Attachment is obsolete: true
Attachment #772156 - Flags: review?(mounir)
Attachment #772156 - Flags: review?(charly.molter)
Attachment #772157 - Flags: review?(mounir)
Attachment #772157 - Flags: review?(charly.molter)
Attachment #772156 - Flags: review?(charly.molter) → review+
Attachment #772157 - Flags: review?(charly.molter) → review+
Attachment #772147 - Flags: review?(mounir) → review+
Attachment #772150 - Flags: review?(mounir) → review+
Attachment #772151 - Flags: review?(mounir) → review+
Attachment #772152 - Flags: review?(mounir) → review+
Attachment #772154 - Flags: review?(mounir) → review+
Attachment #772155 - Flags: review?(mounir) → review+
Attachment #772156 - Flags: review?(mounir) → review+
Attachment #772157 - Flags: review?(mounir) → review+
No problem Mounir! Thanks for the help Mounir and Charly :)
I note that 4a9bbe8cbce6 has the wrong bug # in the commit message.
Ah jeez. Sorry about that Ryan. Is there anything I can do to help fix it?
Given that there are 9 commits in a row and one of them doesn't have the same bug number, I think anyone trying to lookup the bug would understand the mistake easily. The other solution is to backout the commit and push again but that would add some overhead that doesn't seem needed.
Regressions: 1562308
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: