layout/reftests/forms could do with some ordering

RESOLVED FIXED in mozilla25

Status

()

Core
Layout: Form Controls
--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: lahabana, Assigned: reyre)

Tracking

Trunk
mozilla25
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(9 attachments, 9 obsolete attachments)

7.70 KB, patch
mounir
: review+
Details | Diff | Splinter Review
5.37 KB, patch
mounir
: review+
Details | Diff | Splinter Review
2.76 KB, patch
mounir
: review+
Details | Diff | Splinter Review
8.91 KB, patch
mounir
: review+
Details | Diff | Splinter Review
3.80 KB, patch
mounir
: review+
Details | Diff | Splinter Review
6.25 KB, patch
mounir
: review+
Details | Diff | Splinter Review
1.86 KB, patch
mounir
: review+
Details | Diff | Splinter Review
6.63 KB, patch
mounir
: review+
lahabana
: review+
Details | Diff | Splinter Review
22.43 KB, patch
mounir
: review+
lahabana
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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

5 years ago
Whiteboard: [good first bug]
Version: unspecified → Trunk

Updated

5 years ago
Whiteboard: [good first bug] → [good first bug][mentor=lahabana]

Comment 1

5 years ago
@lahabana - i am new here .. could you guide me how to get started on this bug?
(Reporter)

Comment 2

5 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

4 years ago
Is this bug still applicable? If so I can do some work on it.
(Reporter)

Comment 4

4 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

4 years ago
Assignee: nobody → rick.eyre
(Assignee)

Comment 5

4 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

4 years ago
Yes if you don't mind doing separate patches it would be perfect.
(Assignee)

Comment 7

4 years ago
Created attachment 762691 [details] [diff] [review]
Part 1 v1: Move textarea element's to their own folder
Attachment #762691 - Flags: review?(charly.molter)
(Assignee)

Comment 8

4 years ago
Created attachment 762692 [details] [diff] [review]
Part 2 v1: Move select element tests to their own folder
Attachment #762692 - Flags: review?(charly.molter)
(Assignee)

Comment 9

4 years ago
Created attachment 762693 [details] [diff] [review]
Part 3 v1: Move button element tests to their own folder
Attachment #762693 - Flags: review?(charly.molter)
(Assignee)

Comment 10

4 years ago
Created attachment 762694 [details] [diff] [review]
Part 4 v1: Move input checkbox tests to their own folder in the input directory
Attachment #762694 - Flags: review?(charly.molter)
(Assignee)

Comment 11

4 years ago
Created attachment 762695 [details] [diff] [review]
Part 5 v1: Move radio button tests to their own folder
Attachment #762695 - Flags: review?(charly.molter)
(Assignee)

Comment 12

4 years ago
Created attachment 762696 [details] [diff] [review]
Part 6 v1: Move textbox tests to their own directory
Attachment #762696 - Flags: review?(charly.molter)
(Assignee)

Comment 13

4 years ago
Created attachment 762697 [details] [diff] [review]
Part 7 v1: Move legend element tests to their own folder
Attachment #762697 - Flags: review?(charly.molter)
(Assignee)

Comment 14

4 years ago
Created attachment 762698 [details] [diff] [review]
Part 8 v1: Move miscellaneous input tests to the input folder
Attachment #762698 - Flags: review?(charly.molter)
(Reporter)

Updated

4 years ago
Attachment #762691 - Flags: review?(charly.molter) → review+
(Reporter)

Updated

4 years ago
Attachment #762692 - Flags: review?(charly.molter) → review+
(Reporter)

Updated

4 years ago
Attachment #762693 - Flags: review?(charly.molter) → review+
(Reporter)

Updated

4 years ago
Attachment #762694 - Flags: review?(charly.molter) → review+
(Reporter)

Updated

4 years ago
Attachment #762695 - Flags: review?(charly.molter) → review+
(Reporter)

Updated

4 years ago
Attachment #762696 - Flags: review?(charly.molter) → review+
(Reporter)

Updated

4 years ago
Attachment #762697 - Flags: review?(charly.molter) → review+
(Reporter)

Updated

4 years ago
Attachment #762698 - Flags: review?(charly.molter) → review+
(Reporter)

Comment 15

4 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

4 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?
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

4 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

4 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

4 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

4 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

4 years ago
Created attachment 771642 [details] [diff] [review]
Part 1 v2: Move textarea element's to their own folder

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-
(Assignee)

Comment 24

4 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 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.
(Assignee)

Comment 27

4 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

4 years ago
Created attachment 772078 [details] [diff] [review]
Part 1 v2: Move textarea element's to their own folder r=lahabana,mounir

Changed diff format to show renames.

Carrying forward r=lahabana
Attachment #771642 - Attachment is obsolete: true
Attachment #772078 - Flags: review?(mounir)
(Assignee)

Comment 29

4 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.
Attachment #772078 - Flags: review?(mounir) → review+
Rick, could you do the same thing for other patches?
(Assignee)

Comment 31

4 years ago
Will do now Mounir -- was just waiting for your r+ on the first one :)
(Assignee)

Comment 32

4 years ago
Created attachment 772147 [details] [diff] [review]
Part 2 v2: Move select element tests to their own folder

Carrying forward r=lahabana
Attachment #762692 - Attachment is obsolete: true
Attachment #772147 - Flags: review?(mounir)
(Assignee)

Comment 33

4 years ago
Created attachment 772150 [details] [diff] [review]
Part 3 v2: Move button element tests to their own folder

Carrying forward r=lahabana
Attachment #762693 - Attachment is obsolete: true
Attachment #772150 - Flags: review?(mounir)
(Assignee)

Comment 34

4 years ago
Created attachment 772151 [details] [diff] [review]
Part 4 v2: Move input checkbox tests to their own folder in the input directory

Carrying forward r=lahabana
Attachment #762694 - Attachment is obsolete: true
Attachment #772151 - Flags: review?(mounir)
(Assignee)

Comment 35

4 years ago
Created attachment 772152 [details] [diff] [review]
Part 5 v2: Move radio button tests to their own folder

Carrying forward r=lahabana
Attachment #762695 - Attachment is obsolete: true
Attachment #772152 - Flags: review?(mounir)
(Assignee)

Comment 36

4 years ago
Created attachment 772154 [details] [diff] [review]
Part 6 v2: Move textbox tests to their own directory

Carrying forward r=lahabana
Attachment #762696 - Attachment is obsolete: true
Attachment #772154 - Flags: review?(mounir)
(Assignee)

Comment 37

4 years ago
Created attachment 772155 [details] [diff] [review]
Part 7 v2: Move legend element tests to their own folder

Carrying forward r=lahabana
Attachment #762697 - Attachment is obsolete: true
Attachment #772155 - Flags: review?(mounir)
(Assignee)

Comment 38

4 years ago
Created attachment 772156 [details] [diff] [review]
Part 8 v2: Move input tests to their respective folders
Attachment #762698 - Attachment is obsolete: true
Attachment #772156 - Flags: review?(mounir)
Attachment #772156 - Flags: review?(charly.molter)
(Assignee)

Comment 39

4 years ago
Created attachment 772157 [details] [diff] [review]
Part 9 v2: Rename input element tests to remove input-* prefix
Attachment #772157 - Flags: review?(mounir)
Attachment #772157 - Flags: review?(charly.molter)
(Reporter)

Updated

4 years ago
Attachment #772156 - Flags: review?(charly.molter) → review+
(Reporter)

Updated

4 years ago
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+
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
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Assignee)

Comment 41

4 years ago
No problem Mounir!

Thanks for the help Mounir and Charly :)
I note that 4a9bbe8cbce6 has the wrong bug # in the commit message.
(Assignee)

Comment 43

4 years ago
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.
You need to log in before you can comment on or make changes to this bug.