search

AlaSQL / alasql

AlaSQL.js - JavaScript SQL database for browser and Node.js. Handles both traditional relational tables and nested JSON data (NoSQL). Export, store, and import data from localStorage, IndexedDB, or Excel.
http://alasql.org
MIT License
7.02k stars 655 forks source link

58 tests asserted wrongly #1795

Closed rameshrajagopalanbayer closed 1 year ago

rameshrajagopalanbayer commented 1 year ago

As I worked on https://github.com/AlaSQL/alasql/pull/1794 I noticed there are a few specs which follow pattern of assert(res, They seem to pass even when i changed the expected values.

I made a change on the one test in that PR(https://github.com/AlaSQL/alasql/pull/1794/files#diff-45ecd1e6cf603c0d570eae55d8803c4b5af8e3aee3d64d1b061d64814191e146R19).

Should the other specs like shown in this branch https://github.com/AlaSQL/alasql/compare/develop...rameshrajagopalanbayer:alasql:asserts-fix?expand=1 also have the equal or deepEqual added to it,

If this looks to be the case let me know i can make that change and create a PR. Currently i do have 1 spec (uncomment) fail after the change to have equal or deepEqual.

mathiasrw commented 1 year ago

assert with two parameters or assert.equal is used for comparing primitive values or for checking the equality of object references (not their content).

assert.deepEqual, compares the properties and values within the objects recursively to check if they are equal.

So if we have places where we compare objects without using deepEqual, that is a problem

rameshrajagopalanbayer commented 1 year ago

On the equal for example on Test397.js (https://github.com/AlaSQL/alasql/blob/develop/test/test397.js) , I can change the expected value to anything and the test still pass afaict.

Line 22 Fails after expected value change to be incorrect

        var res = alasql('= 256 >> 4');
        assert.equal(res, 56);

vs

Does not fail the test with incorrect expected

        var res = alasql('= 256 >> 4');
        assert(res, 56);

The deep equal on objects is where i first saw the issue for sure. If you can please take a look at above one even, when you get a chance and let me know I can make a PR for above. Also fwiw i tried on node 16 not sure that make any diff and this assert functionality was changed in later versions of node.

mathiasrw commented 1 year ago

This is terrible!

mathiasrw commented 1 year ago

We got 58 examples of this. image

It gave 10 errors after fixing them. I will have to look into this. image

mathiasrw commented 1 year ago

@rameshrajagopalanbayer Thank you for reporting this.