ExploreConsulting / netsuite-fasttrack-toolkit-ss2

NFT for SuiteScript 2.X
74 stars 20 forks source link

Fix unit tests #21

Closed johnogle222 closed 5 years ago

johnogle222 commented 5 years ago

Fixes existing unit tests.

  1. Adds mockrecord.getSublistSubrecord function
  2. Removes test of old immutablejs behavior. It looks like they fixed this over-eager issue as that test is failing on my end
ShawnTalbert commented 5 years ago

That immutablejs test still runs as in on my system (from current master branch) - can you share the test failure?

johnogle222 commented 5 years ago

Sure! After checking out a fresh master from your repo and reinstalling node_modules this is what I get:

  ● ImuutableJS behavior › indirect toString() of Seq causes eager eval

    expect(jest.fn()).toBeCalledTimes(30)

    Expected mock function to have been called 30 times, but it was called five times.

      137 |       // reserialize the entire sequence (1..5)
      138 |       // see next test for workaround
    > 139 |       expect(alwaysTrue).toBeCalledTimes(5 * 6)
          |                          ^
      140 |    })
      141 |
      142 |    test('how to avoid eager eval of Seq', function () {

      at Object.<anonymous> (search.test.ts:139:26)

Additional context:

John-Ogle-MacBook-Pro:netsuite-fasttrack-toolkit-ss2 johno$ npm ls immutable
netsuite-fasttrack-toolkit-ss2@5.1.0 /Users/johno/src/netsuite-fasttrack-toolkit-ss2
└── immutable@3.8.2

John-Ogle-MacBook-Pro:netsuite-fasttrack-toolkit-ss2 johno$ node --version
v12.1.0
ShawnTalbert commented 5 years ago

This might be a nodeJS version issue - I'm still back on 8.15 and a fresh checkout passes that test. I'll update to current LTS and see if the behavior repeats. This is worrying however because the behavior illustrated in this test is what I saw on deployed NS code.... so if it has issues in newer NodeJS we still have the issue in NS I presume.

.. OK just ran this on Node 10.16.0 (Windows/WSL) and it passes.

ShawnTalbert commented 5 years ago

Closing since AFAIK issue remains for code deployed to NS and I cannot reproduce the failing test using latest LTS version of node and a fresh checkout. @johnogle222 let me know if you have any other insights or determine this undesired behavior no longer happens on NS deployed code.

That said, with the recent decoupling of NFT it doesn't have a hard dependency on ImmutableJS. If someone could get IxJS or RxJS working with NFT/NS that would be a fine replacement.

johnogle222 commented 5 years ago

That makes sense to me. If it stays a problem we can rethink it.

There was a separate test this PR fixed though. I reverted the immutable thing, but can you look at the other one?

I opened https://github.com/ExploreConsulting/netsuite-fasttrack-toolkit-ss2/pull/22 with that change