If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Get xpcshell tests for wrapped statements

RESOLVED FIXED

Status

()

Toolkit
Storage
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: adw)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
As per Bug 384454 Comment 5

Right now we only tests strings, and it'd be nice to get the rest tested as well.
(Reporter)

Updated

9 years ago
Whiteboard: [good first bug]
(Assignee)

Updated

9 years ago
Assignee: nobody → adw
(Assignee)

Comment 1

9 years ago
Created attachment 356395 [details] [diff] [review]
patch

Tests int, double, string, boolean, null, and Date object.  A value of each type is inserted into a column of affinity NONE.  Should we be testing every type/column affinity combination?  E.g., int into TEXT, NUMERIC, INTEGER, REAL, NONE, double into TEXT, NUMERIC, INTEGER, REAL, NONE, etc.
Attachment #356395 - Flags: review?(sdwilsh)
(Reporter)

Updated

9 years ago
Attachment #356395 - Flags: review?(sdwilsh) → review+
(Reporter)

Comment 2

9 years ago
Comment on attachment 356395 [details] [diff] [review]
patch

>+/* -*- c-basic-offset: 2 -*- */
If you are going to add modelines, I'd like you to do more complete ones that also cover vim.  See XXX for a good example (note, you'll have to change the first line of the license comment too so you open the comment on the first line, and close it at the end of the license block)

>+function checkVal(actualVal, returnedVal)
javadoc style comments on what this does please

>+function clearTable()
ditto

>+{
>+  var stmt;
>+
>+  stmt = createStatement("DELETE FROM test");
Style is to declare variable at first use, so
let stmt = createStatement("DELETE FROM test");


>+function ensureNumRows(numRows)
javadoc style comments on what this does please

>+  var stmt;
> 
>   stmt = createStatement("SELECT COUNT(*) AS number FROM test");
same as before

>+function printValDesc(val)
javadoc style comments on what this does please

>+  var toSource;
>+
>+  try
>+  {
>+    toSource = val.toSource();
declare with var here - it's function scope! :)

>+function test_binding_params(val)
Since this isn't a "unit test" function anymore, and it's checking conditions, java-doc comment please, and you might want to considering changing its name to something more descriptive

>+  var stmt;
ditto on first use

>+function test_binding_multiple_params(val)
javadoc style comment please

>+{
>+  var stmt;
ditto on declaration

> function run_test()
> {
>+  var vals;
ditto

>+  // Static function JSValStorageStatementBinder in
>+  // storage/src/mozStorageStatementParams.h tells us that the following types
>+  // and only the following types are valid as statement parameters:
>+  vals = [
>+    1337,      // int
>+    3.1337,    // double
>+    "foo",     // string
>+    true,      // boolean
>+    null,      // null
>+    new Date() // Date object
nit: comma after new Date(), so we can add more in the future without modifying irrelevant lines (legal in js arrays)


r=sdwilsh with the above changes, and assuming you've ran the test(s) locally and they pass.  If you want me to look at it again, feel free to request review.

Also, can you please update test_statement_wrapper_automatically.js with these changes?
(Assignee)

Comment 3

9 years ago
Created attachment 356645 [details] [diff] [review]
patch

Updated re: comment 2.  Included updates to test_statement_wrapper_automatically.js as well.

Ideally the new common helper functions would be shared between the two files.  Not sure if it's worth it for these test cases; one option would be to simply combine the two files into one; another would be to do_import_script the helpers from a separate file (I guess).  At this stage I defer to your judgment.
Attachment #356395 - Attachment is obsolete: true
Attachment #356645 - Flags: review?(sdwilsh)
An xpcshell test is run with something like the following commandline:

CMD="./xpcshell -f"
for i in tools/test-harness/xpcshell-simple/head_*.js; do
  CMD="$CMD -f $i"
done
for i in $dir_containing_testfile/head_*.js; do
  CMD="$CMD -f $i"
done
CMD="$CMD -f $testfile"
for i in $dir_containing_testfile/tail_*.js; do
  CMD="$CMD -f $i"
done
for i in tools/test-harness/xpcshell-simple/tail_*.js; do
  CMD="$CMD -f $i"
done

So just dump the functions you want to share into a file named head_<something>.js and they'll be shared across all tests in that directory.
(Reporter)

Comment 5

9 years ago
(In reply to comment #3)
> Ideally the new common helper functions would be shared between the two files. 
> Not sure if it's worth it for these test cases; one option would be to simply
> combine the two files into one; another would be to do_import_script the
> helpers from a separate file (I guess).  At this stage I defer to your
> judgment.
If they were useful to more than just these two files, I'd say pull them out into the head_ file.  Also, the explicit wrapper will be removed eventually since things are automatically wrapped.
(Reporter)

Comment 6

9 years ago
Comment on attachment 356645 [details] [diff] [review]
patch

r=sdwilsh

I have two minor nits (one I missed last time)
1) pArameters to a function should be prefixed with a
2) javadoc params should be formatted like this:
@param aParam
       This text is on a new line
Attachment #356645 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 7

9 years ago
Created attachment 356755 [details] [diff] [review]
for checkin patch

Changes re: comment 6.

Re: comment 4, thanks for the info.  In this case, deferring to comment 5.
Attachment #356645 - Attachment is obsolete: true
Attachment #356755 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Reporter)

Comment 8

9 years ago
http://hg.mozilla.org/mozilla-central/rev/e32e8814d84f
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.