Last Comment Bug 681615 - Move a couple of tests from content/html/content/test to content/html/content/test/forms
: Move a couple of tests from content/html/content/test to content/html/content...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-24 05:11 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-08-25 06:57 PDT (History)
0 users
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (6.22 KB, patch)
2011-08-24 05:11 PDT, :Ms2ger (⌚ UTC+1/+2)
mounir: review+
Details | Diff | Splinter Review
Patch v2 (9.78 KB, patch)
2011-08-24 07:34 PDT, :Ms2ger (⌚ UTC+1/+2)
mounir: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-08-24 05:11:48 PDT
Created attachment 555366 [details] [diff] [review]
Patch v1

I ran into those yesterday; I thought you might this.
Comment 1 Mounir Lamouri (:mounir) 2011-08-24 06:33:34 PDT
Comment on attachment 555366 [details] [diff] [review]
Patch v1

Review of attachment 555366 [details] [diff] [review]:
-----------------------------------------------------------------

r=mounir, if you fix the things I pointed or open follow-ups.

::: content/html/content/test/forms/Makefile.in
@@ +60,5 @@
>  		test_output_element.html \
>  		test_button_attributes_reflection.html \
>  		test_textarea_attributes_reflection.html \
> +		test_validation.html \
> +		test_validation_tooLong.html \

I don't like those tests (yes, I wrote them...). I don't think there is anything we could do about them except review them and see what could be improved/fixed.
BTW, why test_validation_tooLong.html and not test_maxlength_attribute.html?

@@ +62,5 @@
>  		test_textarea_attributes_reflection.html \
> +		test_validation.html \
> +		test_validation_tooLong.html \
> +		test_datalist_options_attribute-1.html \
> +		test_datalist_options_attribute-2.html \

Could you merge them?
Maybe we should make sure the attribute reflection part is in another test too and we should name the test "test_datalist_element.html" because I don't see anything else to test.
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2011-08-24 07:34:42 PDT
Created attachment 555397 [details] [diff] [review]
Patch v2

(In reply to Mounir Lamouri (:volkmar) from comment #1)
> Comment on attachment 555366 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 555366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=mounir, if you fix the things I pointed or open follow-ups.
> 
> ::: content/html/content/test/forms/Makefile.in
> @@ +60,5 @@
> >  		test_output_element.html \
> >  		test_button_attributes_reflection.html \
> >  		test_textarea_attributes_reflection.html \
> > +		test_validation.html \
> > +		test_validation_tooLong.html \
> 
> I don't like those tests (yes, I wrote them...). I don't think there is
> anything we could do about them except review them and see what could be
> improved/fixed.

Bug 681642

> BTW, why test_validation_tooLong.html and not test_maxlength_attribute.html?

Fixed

> @@ +62,5 @@
> >  		test_textarea_attributes_reflection.html \
> > +		test_validation.html \
> > +		test_validation_tooLong.html \
> > +		test_datalist_options_attribute-1.html \
> > +		test_datalist_options_attribute-2.html \
> 
> Could you merge them?

Sure.

> Maybe we should make sure the attribute reflection part is in another test
> too and we should name the test "test_datalist_element.html" because I don't
> see anything else to test.

Which attribute reflection part?
Comment 3 Mounir Lamouri (:mounir) 2011-08-24 08:41:43 PDT
Comment on attachment 555397 [details] [diff] [review]
Patch v2

Review of attachment 555397 [details] [diff] [review]:
-----------------------------------------------------------------

function checkClassesAndAttributes checks if 'options' is an IDL attribute of HTMLDataListElement.
Do we want to keep that or move it to an attribute reflection test?

r=mounir
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2011-08-24 11:45:40 PDT
No, that's just because it's an IDL attribute, nothing to do with reflection.
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2011-08-25 06:57:17 PDT
http://hg.mozilla.org/mozilla-central/rev/80db03383c34

Note You need to log in before you can comment on or make changes to this bug.