1230683 - Cleanup test_storage_connection.js

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The test can use Assert.throws instead of plain try/catch

Comment 1

[Jeffrey Tran]

Attachment: rev1 - Replace try/catch with Assert.throws in test_storage_connection.js

I replaced some try/catch statements with Assert.throws in test_storage_connection.js. Then I ran the test on my local machine and all tests passed.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8714049 [details] [diff] [review]
rev1 - Replace try/catch with Assert.throws in test_storage_connection.js

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

::: storage/test/unit/test_storage_connection.js
@@ +67,5 @@
>  
>  add_task(function* test_createTable_already_created() {
>    var msc = getOpenedDatabase();
>    do_check_true(msc.tableExists("test"));
> +  throws(() => msc.createTable("test", "id INTEGER PRIMARY KEY, name TEXT"), /NS_ERROR_FAILURE/);

A couple things:

1. I know it works, but for readability reasons I prefer if the "Assert." part is explicit, so Assert.throws, so that it's clear where this comes from.

2. please put the error regex on the second line aligned as
Assert.throws(() => {},
              /NS_ERROR_FAILURE/);
since we still have an 80 columns soft-barrier in the coding style

This is valid for all instances where this appears

@@ +267,5 @@
>    let stmt = createStatement("SELECT * FROM test");
>    stmt.executeAsync();
>    stmt.finalize();
>  
> +  let db = getOpenedDatabase();  

trailing spaces here

Comment 3

[Jeffrey Tran]

Attachment: rev2 - Replace try/catch with Assert.throws in test_storage_connection.js

Ok I made changes based on the review feedback of previous patch

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8716383 [details] [diff] [review]
rev2 - Replace try/catch with Assert.throws in test_storage_connection.js

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

thanks you, it looks good to me, I'll push to the TryServer.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4aaaefe703ae

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4aaaefe703ae