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)
Core
Layout: Form Controls
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.
Reporter | ||
Updated•12 years ago
|
Whiteboard: [good first bug]
Updated•12 years ago
|
Version: unspecified → Trunk
Updated•12 years ago
|
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?
Reporter | ||
Comment 2•12 years ago
|
||
(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!
Assignee | ||
Comment 3•11 years ago
|
||
Is this bug still applicable? If so I can do some work on it.
Reporter | ||
Comment 4•11 years ago
|
||
(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 | ||
Updated•11 years ago
|
Assignee: nobody → rick.eyre
Assignee | ||
Comment 5•11 years ago
|
||
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.
Reporter | ||
Comment 6•11 years ago
|
||
Yes if you don't mind doing separate patches it would be perfect.
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #762691 -
Flags: review?(charly.molter)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #762692 -
Flags: review?(charly.molter)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #762693 -
Flags: review?(charly.molter)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #762694 -
Flags: review?(charly.molter)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #762695 -
Flags: review?(charly.molter)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #762696 -
Flags: review?(charly.molter)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #762697 -
Flags: review?(charly.molter)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #762698 -
Flags: review?(charly.molter)
Reporter | ||
Updated•11 years ago
|
Attachment #762691 -
Flags: review?(charly.molter) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #762692 -
Flags: review?(charly.molter) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #762693 -
Flags: review?(charly.molter) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #762694 -
Flags: review?(charly.molter) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #762695 -
Flags: review?(charly.molter) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #762696 -
Flags: review?(charly.molter) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #762697 -
Flags: review?(charly.molter) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #762698 -
Flags: review?(charly.molter) → review+
Reporter | ||
Comment 15•11 years ago
|
||
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!
Assignee | ||
Comment 16•11 years ago
|
||
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?
Comment 17•11 years ago
|
||
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.
Assignee | ||
Comment 18•11 years ago
|
||
Hmm, I guess git records renames different than mercurial? I'll fix that up and try to get it back to you soon.
Reporter | ||
Comment 19•11 years ago
|
||
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
Assignee | ||
Comment 20•11 years ago
|
||
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.
Assignee | ||
Comment 21•11 years ago
|
||
Sorry this is taking me a while. I'm still working on it and will have a patch this week.
Assignee | ||
Comment 22•11 years ago
|
||
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 23•11 years ago
|
||
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-
Assignee | ||
Comment 24•11 years ago
|
||
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 25•11 years ago
|
||
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+
Comment 26•11 years ago
|
||
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.
Assignee | ||
Comment 27•11 years ago
|
||
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.
Assignee | ||
Comment 28•11 years ago
|
||
Changed diff format to show renames.
Carrying forward r=lahabana
Attachment #771642 -
Attachment is obsolete: true
Attachment #772078 -
Flags: review?(mounir)
Assignee | ||
Comment 29•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #772078 -
Flags: review?(mounir) → review+
Comment 30•11 years ago
|
||
Rick, could you do the same thing for other patches?
Assignee | ||
Comment 31•11 years ago
|
||
Will do now Mounir -- was just waiting for your r+ on the first one :)
Assignee | ||
Comment 32•11 years ago
|
||
Carrying forward r=lahabana
Attachment #762692 -
Attachment is obsolete: true
Attachment #772147 -
Flags: review?(mounir)
Assignee | ||
Comment 33•11 years ago
|
||
Carrying forward r=lahabana
Attachment #762693 -
Attachment is obsolete: true
Attachment #772150 -
Flags: review?(mounir)
Assignee | ||
Comment 34•11 years ago
|
||
Carrying forward r=lahabana
Attachment #762694 -
Attachment is obsolete: true
Attachment #772151 -
Flags: review?(mounir)
Assignee | ||
Comment 35•11 years ago
|
||
Carrying forward r=lahabana
Attachment #762695 -
Attachment is obsolete: true
Attachment #772152 -
Flags: review?(mounir)
Assignee | ||
Comment 36•11 years ago
|
||
Carrying forward r=lahabana
Attachment #762696 -
Attachment is obsolete: true
Attachment #772154 -
Flags: review?(mounir)
Assignee | ||
Comment 37•11 years ago
|
||
Carrying forward r=lahabana
Attachment #762697 -
Attachment is obsolete: true
Attachment #772155 -
Flags: review?(mounir)
Assignee | ||
Comment 38•11 years ago
|
||
Attachment #762698 -
Attachment is obsolete: true
Attachment #772156 -
Flags: review?(mounir)
Attachment #772156 -
Flags: review?(charly.molter)
Assignee | ||
Comment 39•11 years ago
|
||
Attachment #772157 -
Flags: review?(mounir)
Attachment #772157 -
Flags: review?(charly.molter)
Reporter | ||
Updated•11 years ago
|
Attachment #772156 -
Flags: review?(charly.molter) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #772157 -
Flags: review?(charly.molter) → review+
Updated•11 years ago
|
Attachment #772147 -
Flags: review?(mounir) → review+
Updated•11 years ago
|
Attachment #772150 -
Flags: review?(mounir) → review+
Updated•11 years ago
|
Attachment #772151 -
Flags: review?(mounir) → review+
Updated•11 years ago
|
Attachment #772152 -
Flags: review?(mounir) → review+
Updated•11 years ago
|
Attachment #772154 -
Flags: review?(mounir) → review+
Updated•11 years ago
|
Attachment #772155 -
Flags: review?(mounir) → review+
Updated•11 years ago
|
Attachment #772156 -
Flags: review?(mounir) → review+
Updated•11 years ago
|
Attachment #772157 -
Flags: review?(mounir) → review+
Comment 40•11 years ago
|
||
Thanks for those patches Rick :)
remote: https://hg.mozilla.org/mozilla-central/rev/d95765af8899
remote: https://hg.mozilla.org/mozilla-central/rev/c41a7e8f39e8
remote: https://hg.mozilla.org/mozilla-central/rev/d0690bff45c8
remote: https://hg.mozilla.org/mozilla-central/rev/4a9bbe8cbce6
remote: https://hg.mozilla.org/mozilla-central/rev/5c3dc97d8446
remote: https://hg.mozilla.org/mozilla-central/rev/d0c09f439ec5
remote: https://hg.mozilla.org/mozilla-central/rev/9d048f89d3a4
remote: https://hg.mozilla.org/mozilla-central/rev/6e7ef4bcd7b2
remote: https://hg.mozilla.org/mozilla-central/rev/04d8c309fe72
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Comment 41•11 years ago
|
||
No problem Mounir!
Thanks for the help Mounir and Charly :)
Comment 42•11 years ago
|
||
I note that 4a9bbe8cbce6 has the wrong bug # in the commit message.
Assignee | ||
Comment 43•11 years ago
|
||
Ah jeez. Sorry about that Ryan.
Is there anything I can do to help fix it?
Comment 44•11 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•