node-red-node-test-helper icon indicating copy to clipboard operation
node-red-node-test-helper copied to clipboard

Pass settings as optional parameter to helper.load()

Open cdlans opened this issue 7 years ago • 8 comments

At the moment it is impossible to test a flow which has a function node, which loads a module with a call to global.get(). (See the documentation on loading modules in function nodes.)

This PR adds the functionality, such that the test-helper loads a settings.js file from the current working directory (if present). Thus it will be possible to specify modules with settings.functionGlobalContext.

cdlans avatar Sep 14 '18 22:09 cdlans

Hi - as per my comment on #21, I don't think this module should be reading files from disk. That doesn't really fit with the unit-testing role this module provides.

The appropriate fix will be for the API this module exposes to allow you to pass in a settings object. You can then obtain that settings object in your own test code however you want.

knolleary avatar Sep 14 '18 22:09 knolleary

Hi Nick,

Updated the PR. The helper.load() function now takes an optional parameter testSettings. I added the parameter in such a way that it is backwards compatible, i.e. the function can still be called without supplying testSettings and/or testCredentials.

Casper

cdlans avatar Sep 16 '18 15:09 cdlans

Hi Nick,

The release of node-red v0.19.4 seems to have broken my feature.

TypeError: Cannot redefine property: get
      at Function.defineProperties (<anonymous>)
      at createContext (node_modules\node-red\red\runtime\nodes\context\index.js:224:12)
      at Object.init (node_modules\node-red\red\runtime\nodes\context\index.js:56:26)
      at Object.init (node_modules\node-red\red\runtime\nodes\index.js:101:13)
      at NodeTestHelper.load (...\node-red-node-test-helper\index.js:185:18)

Any idea why?

cdlans avatar Sep 18 '18 09:09 cdlans

In 0.19.4 we made the context.get/set functions non-enumerable properties so they can't be accidentally deleted - a user had a flow that was doing so and breaking the internals of Node-RED.

What is the Function node you are testing doing?

knolleary avatar Sep 18 '18 09:09 knolleary

The function node has a dependency on moment.js, which is passed in through settings.functionGlobalContext. To write tests for the flow containing this node, I had to adapt helper.load() to accept the settings as parameter.

I noticed today that with the 0.19.4 version, my first test still passes, but the following tests fail. My suspicion is that somehow the settings are not cleared in between tests, and that the second test tries to overwrite the settings, leading to the error above.

Haven't had much time to look into it today, but was just wondering: Why is this._context.clean({allNodes:[]}) called with an empty array? Shouldn't it have the flow as parameter, so it can clean the context of the nodes?

cdlans avatar Sep 18 '18 18:09 cdlans

The clean function is used to remove contexts that are no longer required. The allNodes array is a list of nodes that are still active - ie the ones whose contexts must be left alone. So by passing in an empty array, it will remove all existing contexts.

However - global context is a special case - it never gets cleaned. So I suspect you are right that your changes are causing to reinitialise the existing global context object which it doesn't like.

I'll try to recreate based on that theory - although it won't be for a day or two.

knolleary avatar Sep 18 '18 22:09 knolleary

I have recreated the issue. The first test passes, the next 4 fail. If the node-red dependency is downgraded to 0.19.3, all tests pass.

Thanks for taking the time to look into this. I really appreciate it.

cdlans avatar Sep 21 '18 21:09 cdlans

By the way, why is the global context never cleaned? What would be the solution to my problem? Should I somehow change the helper to accept the settings.functionGlobalContext in a before() hook, instead of a beforeEach() hook? Or should there be a special cleanGlobal() in node-red, that cleans the global context, but should only be used for tests?

cdlans avatar Sep 22 '18 15:09 cdlans