phetsims / query-string-machine

Query String Machine is a query string parser that supports type coercion, default values & validation. No dependencies.
MIT License
3 stars 3 forks source link

testQueryStringMachine relies on JSON.stringify string tests #24

Closed pixelzoom closed 7 years ago

pixelzoom commented 7 years ago

testQueryStringMachine.js has a serious problem. It's fundamentally based on comparing the results of JSON.stringify, which is not guaranteed to return results in any predictable order.

Here's the problematic code:

  var test = function( testName, queryString, expected, schema ) {
    var queryParameters = QueryStringMachine.getAllForString( queryString, schema );
    var a = JSON.stringify( queryParameters );
    var b = JSON.stringify( expected );
    if ( a !== b ) {
      console.log( 'Mismatch: ' + a + ' vs. ' + b );
    }
    testAssert( a === b, testName );
    console.log( testName + ' passed' );
  };

Even if JSON.stringify did guarantee order, there's another problem with this approach. It requires expected and schema to have all of their fields listed in the same order.

pixelzoom commented 7 years ago

I removed the JSON.stringify comparison and did a property-by-property comparison of actual and expected results, including a deep comparison of arrays. This is going to be problematic for arrays of non-integral types, and for type 'custom'.

And there are other problems here. Verifying that something fails when it's supposed to is cumbersome, and doesn't fit with the "passing" tests. And while this started out as 1 test schema, that seems to be falling apart - people have obviously been tacking tests on that have their own schemas.

Imo, this is complicated enough that we should consider using qunit.

pixelzoom commented 7 years ago

@samreid How do you want to proceed with this test harness?

samreid commented 7 years ago

test-query-string-machine now uses QUnit, including assert.throws and assert.deepEquals, it is much nicer now.

@pixelzoom would you like to take a look?

pixelzoom commented 7 years ago

👍 Closing.