1230685 - Remove custom test runner from test_storage_value_array.js and test_unicode.js

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Marco Bonardo [:mak]]

The tests should use add_test instead of a custom test runner

Comment 1

[chaithanya]

Hi,
   I am a beginner and would like to work on this bug. Can you please tell me how to go ahead with this bug?

Comment 2

[Jeffrey Tran]

Attachment: rev1 - Replaced function declarations with add_task

Hi, I made this patch based on what changes I think are needed, but I'm not completely sure. Also, the tests didn't seem to run correctly when I used add_test so instead I used add_task

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8714052 [details] [diff] [review]
rev1 - Replaced function declarations with add_task

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

please, rememeber to append the reviewer at the end of the commit message as ". r=mak"

Likely add_test didn't work as you were expecting cause it still requires to call run_next_test() at the end of the test, so either

add_task(function* myTest() {
  ...
});

or

add_test(function myTest() {
  ...
  run_next_test();
});

We don't have a strict requirement whether to use one or the other one, usually synchronous tests keep using add_test, but both will work.

Note the comments here are valid for both tests, even if I only commented on one of them.

::: storage/test/unit/test_storage_value_array.js
@@ +182,1 @@
>  }

This boilerplate is no more needed.

@@ -206,5 @@
> -    tests[i]();
> -  }
> -
> -  cleanup();
> -}

you are not invoking cleanup() anymore at the end of the test this way.
You can use do_register_cleanup(cleanup); inside the setup() function

Comment 4

[Marco Bonardo [:mak]]

The review context is broken, the no more needed boilerplate is:

function run_test() {
  run_next_test();
}

Comment 5

[Jeffrey Tran]

Attachment: rev2 - Replaced function declarations with add_task

Ok I made changes based on feedback

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8716389 [details] [diff] [review]
rev2 - Replaced function declarations with add_task

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

r=me with the following comments addressed

::: storage/test/unit/test_storage_value_array.js
@@ +21,5 @@
>    stmt.execute();
>  
>    stmt.reset();
>    stmt.finalize();
> +  

some trailing spaces here

::: storage/test/unit/test_unicode.js
@@ +23,5 @@
>    stmt.bindByIndex(0, LATIN1_ae);
>    stmt.bindByIndex(1, 4);
>    stmt.execute();
>    stmt.finalize();
> +  

some trailing spaces

Comment 7

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab419d75f5e0

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6e66d7b4e957

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e66d7b4e957