Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-21087

Some uses of assert.throws() always pass

XMLWordPrintableJSON

    • Fully Compatible
    • ALL
    • TIG 2016-10-10, TIG 2016-10-31

      When assert.throws() calls the given function, this is not set. This means functions that rely on this being set (eg. most member functions of DB or DBCollection) are highly likely to throw an exception when this turns out to be null. This makes the assert.throws() test succeed, but not because the intended assertion was thrown.

      For example, consider from jstests/sharding/authmr.js:69:

      assert.throws(test2DB.foo.count);

      However, the count function has calls to this.find and this.getName, which cause an exception:

      > db.foo.count()
8
> if (assert.throws(db.foo.count)) print("success")
success
> assert.throws(db.foo.count)
TypeError: this.find is not a function

      There are up to 625 uses of assert.throws, of which up to 56 may be affected:

      $ grep -nr 'assert\.throws' jstests | grep -v 'assert\.throws\(\.automsg\)\?\s*(\s*function\s*(' | grep 'assert\.throws'
jstests/dur/diskfull.js:105:    assert.throws( work, null, "no exception thrown when exceeding disk capacity" );
jstests/ssl/libs/ssl_helpers.js:44:    assert.throws(load,[replSetTestFile],
jstests/sharding/replmonitor_bad_seed.js:46:assert.throws(verifyInsert);
jstests/sharding/version2.js:52:assert.throws( simpleFindOne , [] , "should complain about not in sharded mode 1" );
jstests/sharding/authmr.js:69:        assert.throws(test2DB.foo.count);
jstests/sharding/authwhere.js:66:        assert.throws(test2DB.foo.count);
jstests/sharding/authwhere.js:69:        assert.throws(test1DB.foo.count, ["db.getSiblingDB('test2').foo.count() == 1"]);
jstests/sharding/authwhere.js:72:        assert.throws(test1DB.foo.count, ["db.foo.insert({b: 1})"]);
jstests/core/counta.js:11:assert.throws(
jstests/core/geo_s2disjoint_holes.js:35:print(assert.throws(
jstests/core/geo_s2disjoint_holes.js:42:print(assert.throws(
jstests/core/distinct4.js:18:    assert.throws( t.distinct, [{a:1}] );
jstests/core/distinct4.js:28:    assert.throws( t.distinct, ['a', '1'] );
jstests/core/basic3.js:21:assert.throws(doBadSave, [{"a.b":5}], ". in names aren't allowed doesn't work");
jstests/core/basic3.js:23:assert.throws(doBadSave,
jstests/core/basic3.js:30:assert.throws(doBadUpdate, [{a:0}, { "b.b" : 1 }],
jstests/core/basic3.js:34:assert.throws(doBadUpdate, [{a:10}, { c: {"b.b" : 1 }}],
jstests/core/basic3.js:42:assert.throws(doBadUpdate, [{a:0}, { "":{"b.b" : 1} }],
jstests/core/indexes_on_indexes.js:16:    assert.throws(t.system.indexes.createIndex({_id:1}));
jstests/core/indexes_on_indexes.js:21:    assert.throws(t.system.indexes.insert({ v:1,
jstests/core/basic9.js:16:assert.throws(doBadSave, [{$foo:5}], "key names aren't allowed to start with $ doesn't work");
jstests/core/basic9.js:17:assert.throws(doBadSave,
jstests/core/shellstartparallel.js:4:assert.throws(f);
jstests/core/error2.js:9:assert.throws(
jstests/core/error2.js:16:assert.throws(
jstests/core/fts_partition1.js:13:assert.throws(t.find( { "$text": { "$search" : "foo" } } ));
jstests/core/cappeda.js:23://assert.throws( q , [] , "A1" );
jstests/core/cappeda.js:24://assert.throws( u , [] , "B1" );
jstests/core/fts_mix.js:72:assert.throws(tc.find( { "$text": { "$search": "member", $language: "spanglish" } } ));
jstests/core/geo_2d_with_geojson_point.js:14:print(assert.throws(
jstests/core/push_sort.js:65:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [ 2 ], $slice:-2, $sort:{a:1} } } } ) )
jstests/core/push_sort.js:68:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2},1], $slice:-2, $sort:{a:1} } } }))
jstests/core/push_sort.js:71:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort:{} } } } ) )
jstests/core/push_sort.js:74:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:2, $sort: {a:1} } } }))
jstests/core/push_sort.js:77:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2.1, $sort: {a:1} } }}))
jstests/core/push_sort.js:80:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort: {a:-2} } } }))
jstests/core/push_sort.js:84:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort: 1 } } } ) )
jstests/core/push_sort.js:87:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort: {'a.':1} }}}))
jstests/core/push_sort.js:90:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $sort: {'':1} } } }))
jstests/core/push_sort.js:94:assert.throws( t.update( {_id:100}, { $push: { x: { $each: [{a:2}], $slice:-2, $xxx: {s:1} } } } ) )
jstests/core/push_sort.js:101:assert.throws(t.update(
jstests/core/ord.js:34:assert.throws( c.next() );
jstests/core/invalid_db_name.js:13:        assert.throws(doWrite);
jstests/concurrency/fsm_selftests.js:19:    // when assert.throws executes
jstests/replsets/maintenance.js:74:var ex = assert.throws(
jstests/replsets/server8070.js:104:// the sleep 30sec is a hold over from the unsafe assert.throws(assert.soon())
jstests/replsets/stepdown3.js:39:        throw new Error("satisfy assert.throws()");
jstests/replsets/stepdown3.js:44:var result = assert.throws(gleFunction);
jstests/multiVersion/libs/verify_collection_data.js:145:    assert.throws(myValidator.validateCollectionData, [collection], "Validation function should have thrown since we modified the collection");
jstests/multiVersion/libs/verify_collection_data.js:153:    assert.throws(myValidator.validateCollectionData, [collection], "Validation function should have thrown since we modified the collection");
jstests/multiVersion/libs/verify_collection_data.js:167:    assert.throws(myValidator.validateCollectionData, [collection], "Validation function should have thrown since we modified the collection");
jstests/multiVersion/libs/verify_collection_data.js:175:    assert.throws(myValidator.validateCollectionData, [collection], "Validation function should have thrown since we modified the collection");
jstests/noPassthroughWithMongod/log_component_helpers.js:35:    assert.throws(mongo.setLogLevel, [2, 24]);
jstests/noPassthroughWithMongod/log_component_helpers.js:36:    assert.throws(db.setLogLevel, [2, ["array", "not.allowed"]]);
jstests/noPassthroughWithMongod/no_balance_collection.js:6:assert.throws(sh.disableBalancing, [], "sh.disableBalancing requires a collection");
jstests/noPassthroughWithMongod/no_balance_collection.js:7:assert.throws(sh.enableBalancing, [], "sh.enableBalancing requires a collection");

      Possible solutions:

      1. Make assert.throws accept a this parameter which gets passed on to func.apply.
      2. Make callers of assert.throws declare upfront a substring that the assertion should contain. This can be achieved manually by using String.prototype.includes on the result of assert.throws, but there's no way to enforce that callers of assert.throws do this (and it's also complicated because assert.throws can return undefined or a non-string). By contrast, assert.throws(func, params, requiredAssertion, msg) would only pass if func(params) throws an exception and the exception contains requiredAssertion as a substring.
      3. Instead of passing member functions directly to assert.throws, wrap them in an anonymous function (variables/functions will still be available via a closure) which calls the member normally, thereby ensuring that this is set as expected. 
        > if (assert.throws(function () { db.foo.count(); })) print("success")
assert: did not throw exception: undefined
doassert@src/mongo/shell/assert.js:15:14
assert.throws@src/mongo/shell/assert.js:229:5
@(shell):1:5

2015-10-23T15:15:48.446+1100 E QUERY    [thread1] Error: did not throw exception: undefined :
doassert@src/mongo/shell/assert.js:15:14
assert.throws@src/mongo/shell/assert.js:229:5
@(shell):1:5
      4. Notice (somehow) if the assertion fails due to a problem with this, and do not consider that a valid assertion (for the purposes of assert.throws passing). Eg. if it matches 
        ^TypeError: this\.[^ ]\+ is not a function$

      #1 and #2 are probably better for preventing re-occurrences of the problem being introduced in the future, but will involve widespread changes, while #3 is likely to be much quicker/easier. #4 makes me nervous about falsely flagging assertions that happen to legitimately involve this, as well as needing to enumerate all the ways that this misuse of assert.throws can manifest.

            Assignee:
            robert.guo@mongodb.com Robert Guo (Inactive)
            Reporter:
            kevin.pulo@mongodb.com Kevin Pulo
            Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

              Created:
              Updated:
              Resolved: