Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass settings as optional parameter to helper.load() #23

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cdlans
Copy link

@cdlans cdlans commented Sep 14, 2018

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.

@knolleary
Copy link
Member

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.

@cdlans cdlans changed the title Read settings.js from current working directory Pass settings as optional parameter to helper.load() Sep 16, 2018
@cdlans
Copy link
Author

cdlans commented Sep 16, 2018

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
Copy link
Author

cdlans commented Sep 18, 2018

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?

@knolleary
Copy link
Member

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?

@cdlans
Copy link
Author

cdlans commented Sep 18, 2018

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?

@knolleary
Copy link
Member

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.

@cdlans
Copy link
Author

cdlans commented Sep 21, 2018

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
Copy link
Author

cdlans commented Sep 22, 2018

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants