Add property based tests to assess load reverses dump

[dubzzz]

No description provided.

[puzrin]

I don't understand why this test is needed. Could you explain?

[dubzzz]

@puzrin

The idea of this test is to cover (and maybe discover) edge-cases of the js-yaml package we have not think about in the already existing unit-tests. By definition, units cannot cover the range of all possible failures and might miss some complex corner cases. They are based on fixed inputs and expect fixed outputs.

The suggested and complementary approach suggested here, is to add some properties (or contracts) we want to confirm on the package. For the moment the one I introduced check that for any valid object the yaml produced by js-yaml can be read by itself and return the original object.

The strength of property based testing approach is that it does not limit itself to a pre-defined subset of possible entries but goes far beyond by building whatever entry we considered valid. In this example, I defined a valid entry as being all objects whose keys are strings and values are booleans, numbers, strings, nulls, arrays of values or sub-objects. If for any reasons, one of the objects built by the framework made the property failed, the framework will try to reduce it to the minimal object (less keys, smaller values...).

[puzrin]

IMHO, in existing context, this looks like adding code "just for fun" - no examples that such tests could help in the past and how those will help in future. I prefer to avoid unnecessary code when possible - more easy to maintain then.

[dubzzz]

I found a bug in the code of js-yaml using this technic. Indeed js-yaml can produce illegal unicode files. It accepts incomplete surrogate pairs as input.

[dubzzz]

For the above comment, you can try to build a YAML file from {'\ud800' :''}. It generates an invalid file assuming unicode.

[dubzzz]

@puzrin Forget about the encoding issue I reported. I enhanced the scope of my property based tests in order to find real issues and found some quite interesting ones when playing on options used by safeDump.

It appears that some yaml values generated by safeDump cannot be read properly by safeLoad.
The following code raises a YAMLException: missed comma between flow collection entries at line 1, column 8

yaml.safeLoad(yaml.safeDump({"":[{"\u0000":""}]},{"condenseFlow":true,"flowLevel":1}));

@puzrin Can you confirm that this is a real bug?

More on this test at https://runkit.com/dubzzz/multiple-properties-for-js-yaml
I really believe that it can find issues not covered by any tests. If you agree it would be nice that we update this pull request in order to cover the edge cases I developed in runkit example.

[puzrin]

condenceFlow and flowLevel can not be used in the same time. IMHO it doesn't worth spend time for all possible bad combinations of options.

[dubzzz]

@puzrin
I tried to extend a bit the scope of the cases covered by my tests (cf. previous runkit link). There is an inconsistency in the behaviour seen when using custom styles for integer type.

const options = {styles:{'!!int':'binary'}};
yaml.safeLoad(yaml.safeDump({toto: 10}, options)); // produces: {toto: 10}
yaml.safeLoad(yaml.safeDump({toto: -10}, options)); // produces: {toto: "0b-1010"}

When negative the integers values produced by js-yaml are read as strings (whereas they are read as integers for positive integers).
Please note that the source code of the test in runkit is a draft and can easily be enhanced.

[dubzzz]

The right negative binary for -10 should have been -0b1010 not 0b-1010

[puzrin]

I agree, that's a real bug of int representer https://github.com/nodeca/js-yaml/blob/master/lib/js-yaml/type/int.js#L159-L164.

Question is - do we need this code in core to maintain forever, or it's a one-time action.

[dubzzz]

I would say that yes, we should keep some kind of test like this. They ensure non regression and can cover configurations yet uncovered by unit tests. Basically, no one can confirm that no other combination of settings or refactoring would break the code in the future. So yes, I believe it can still be useful to have it.

I am currently trying to revamp the code of the pull request in order to provide a more maintainable code.

[puzrin]

Ок. Put it all into 25-dumper-fuzzy.js and i'll merge it.

[dubzzz]

Just updated the pull request. Expected failure due to #399

You can see that in the logs of the job:

1) Properties Load from dumped should be the original object:
     Property failed after 1 tests (seed: 1519328411511): [{"":[-1]},{"styles":{"!!int":"binary"}}]
Got error: AssertionError [ERR_ASSERTION]: { '': [ '0b-1' ] } deepStrictEqual { '': [ -1 ] }

Please keep me updated if you want a change in the code.

[puzrin]

IMHO it's better to put everything into one file, as i said - more easy to maintain. No need to create complicated files structure in advance.

[dubzzz]

Ok, I will update my change

[puzrin]

Could you put here content of ./properties/identity.js instead of files load? You do too complicated load for single test that will never be extended (with high probability).

[dubzzz]

Update done, sorry for the delay

[puzrin]

Thanks!