Re: Test Case Approval Request

On 04/05/10 06:38, Kris Krueger wrote:
> Attached is an update to the test case that I'd like to see approved.
> If you want to run this test for interoperability then Remove the '.txt' and place domtestcase.js at ../common.
>
> Updates
>   * Change title/description, from interface to attribute (annevk@opera.com  feedback)
>   * Change doctype to html5 (tross@microsoft.com  feedback)
>   * Enable more tests via code resuse, e.g. be more data driven and run test in a loop (annevk@opera.com  feedback)
>   * Update to use strict equality for assertEquals (jgraham@opera.com)
>   * Create common folder to hold domtestcase.js (krisk@microsoft.com  feedback)
>
> If you don't agree now is the time to speak up, in the list or at the conf call in the morning.

Having finally got around to looking at this:

a) Each test tests too much. I'd much rather have one test for each 
thing (i.e., one for reading getAttribute('href'), one for 
setAttribute('href', ''), and one for set followed by get). Also, how is 
this a test for HTML5? Behaviour of getAttribute, setAttribute, and 
removeAttribute that the test tests is defined in DOM Level 3 Core; the 
only part of the behaviour defined in HTML5 is case-insensitivity.

b) There's quite a lot of properties on the global object used, which I 
don't particularly like. I see no reason not to declare them as 
variables in the current scope (i.e., with "var foo").

c) Although technically fine for something in an HTML5 test suite, 
relying upon innerHTML seems risky when it isn't actually trying to test 
fragment parsing/serialization. It also complicates things when trying 
to create XHTML versions of the current tests in existing browsers (of 
which only Opera supports innerHTML in the XHTML case). I'd much rather 
just use Element.textContent, though I can see an argument for only 
relying upon DOM Level 1 Core as much as possible (i.e., for all things 
that don't need namespaces) and using Text.data instead.

d) In similar vein to a), it seems bad to have the same description for 
all asserts within a single test.

e) Using console.log (which is completely non-standard) within the catch 
block (and hence throwing an uncaught exception if it is not supported) 
seems bad, to say the least.

-- 
Geoffrey Sneddon — Opera Software
<http://gsnedders.com/>
<http://www.opera.com/>

Received on Tuesday, 4 May 2010 14:43:27 UTC