785821 - layout/reftests/forms could do with some ordering

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, enhancement)

Description

[Charly Molter :lahabana]

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.

Comment 1

[rb]

@lahabana - i am new here .. could you guide me how to get started on this bug?

Comment 2

[Charly Molter :lahabana]

(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!

Comment 3

[Rick Eyre (:reyre)]

Is this bug still applicable? If so I can do some work on it.

Comment 4

[Charly Molter :lahabana]

(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.

Comment 5

[Rick Eyre (:reyre)]

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.

Comment 6

[Charly Molter :lahabana]

Yes if you don't mind doing separate patches it would be perfect.

Comment 7

[Rick Eyre (:reyre)]

Attachment: Part 1 v1: Move textarea element's to their own folder

Comment 8

[Rick Eyre (:reyre)]

Attachment: Part 2 v1: Move select element tests to their own folder

Comment 9

[Rick Eyre (:reyre)]

Attachment: Part 3 v1: Move button element tests to their own folder

Comment 10

[Rick Eyre (:reyre)]

Attachment: Part 4 v1: Move input checkbox tests to their own folder in the input directory

Comment 11

[Rick Eyre (:reyre)]

Attachment: Part 5 v1: Move radio button tests to their own folder

Comment 12

[Rick Eyre (:reyre)]

Attachment: Part 6 v1: Move textbox tests to their own directory

Comment 13

[Rick Eyre (:reyre)]

Attachment: Part 7 v1: Move legend element tests to their own folder

Comment 14

[Rick Eyre (:reyre)]

Attachment: Part 8 v1: Move miscellaneous input tests to the input folder

Comment 15

[Charly Molter :lahabana]

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!

Comment 16

[Rick Eyre (:reyre)]

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

[Mounir Lamouri (:mounir)]

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.

Comment 18

[Rick Eyre (:reyre)]

Hmm, I guess git records renames different than mercurial? I'll fix that up and try to get it back to you soon.

Comment 19

[Charly Molter :lahabana]

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

Comment 20

[Rick Eyre (:reyre)]

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.

Comment 21

[Rick Eyre (:reyre)]

Sorry this is taking me a while. I'm still working on it and will have a patch this week.

Comment 22

[Rick Eyre (:reyre)]

Attachment: 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?

Comment 23

[Mounir Lamouri (: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.

Comment 24

[Rick Eyre (:reyre)]

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

[Mounir Lamouri (:mounir)]

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.

Comment 26

[Mounir Lamouri (:mounir)]

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.

Comment 27

[Rick Eyre (:reyre)]

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.

Comment 28

[Rick Eyre (:reyre)]

Attachment: Part 1 v2: Move textarea element's to their own folder r=lahabana,mounir

Changed diff format to show renames.

Carrying forward r=lahabana

Comment 29

[Rick Eyre (:reyre)]

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.

Comment 30

[Mounir Lamouri (:mounir)]

Rick, could you do the same thing for other patches?

Comment 31

[Rick Eyre (:reyre)]

Will do now Mounir -- was just waiting for your r+ on the first one :)

Comment 32

[Rick Eyre (:reyre)]

Attachment: Part 2 v2: Move select element tests to their own folder

Carrying forward r=lahabana

Comment 33

[Rick Eyre (:reyre)]

Attachment: Part 3 v2: Move button element tests to their own folder

Carrying forward r=lahabana

Comment 34

[Rick Eyre (:reyre)]

Attachment: Part 4 v2: Move input checkbox tests to their own folder in the input directory

Carrying forward r=lahabana

Comment 35

[Rick Eyre (:reyre)]

Attachment: Part 5 v2: Move radio button tests to their own folder

Carrying forward r=lahabana

Comment 36

[Rick Eyre (:reyre)]

Attachment: Part 6 v2: Move textbox tests to their own directory

Carrying forward r=lahabana

Comment 37

[Rick Eyre (:reyre)]

Attachment: Part 7 v2: Move legend element tests to their own folder

Carrying forward r=lahabana

Comment 38

[Rick Eyre (:reyre)]

Attachment: Part 8 v2: Move input tests to their respective folders

Comment 39

[Rick Eyre (:reyre)]

Attachment: Part 9 v2: Rename input element tests to remove input-* prefix

Comment 40

[Mounir Lamouri (:mounir)]

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

Comment 41

[Rick Eyre (:reyre)]

No problem Mounir!

Thanks for the help Mounir and Charly :)

Comment 42

[Ryan VanderMeulen [:RyanVM]]

I note that 4a9bbe8cbce6 has the wrong bug # in the commit message.

Comment 43

[Rick Eyre (:reyre)]

Ah jeez. Sorry about that Ryan.

Is there anything I can do to help fix it?

Comment 44

[Mounir Lamouri (:mounir)]

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.