-
Notifications
You must be signed in to change notification settings - Fork 4
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
initial prototype of flow testing plugin #4
base: main
Are you sure you want to change the base?
initial prototype of flow testing plugin #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making a start on this. Some initial comments here - I haven't dug into the UI bits too much, more on the overall approach.
@@ -0,0 +1,61 @@ | |||
<!-- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not add the license header. We're moving away from doing that as the consensus is having the top-level LICENSE file is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed license from code.
src/flow-tester.js
Outdated
checks = []; | ||
checkSend = {}; | ||
|
||
RED.hooks.add("onSend.flow-test", (events, done) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to discuss how the runtime side is going to work. I don't believe runtime hooks are enough here - although they will play a part.
I would like to understand what you are planning here, or if this is just a place holder to show the concept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this runtime hook is to insert code in the middle to inspect the message.
In the initial design, the idea of using a pluggable message router was proposed.
src/flow-tester.html
Outdated
|
||
function invokeAction(kind, opt) { | ||
return $.ajax({ | ||
url: "/flow-tester/executeAction/"+ kind, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not start the url with /
- otherwise it will not work if the user changes httpAdminRoot
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the /
.
src/flow-tester.html
Outdated
RED.events.on("nodes:remove", handleNodeRemove); | ||
RED.events.on("nodes:change", handleNodeChange); | ||
|
||
RED.events.on("nodes:dialog-open", handleNodeDialogOpen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the original design discussion, we need a proper design for how plugins can contribute panes to the editor dialog. I've not seen any discussion on that.
I don't think this event mechanism is the right approach. I have something in mind and I will get that written up in the next few days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I think being able to hook various dialogs would be useful also in other cases.
}, | ||
onadd: () => { | ||
const routeAuthHandler = RED.auth.needsPermission("flow-tester.write"); | ||
RED.httpAdmin.post( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have some documentation for each of these actions to clearly define what it is meant to do - even if not yet implemented or just a placeholder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added simple descriptions.
I have committed the current flow-tester code.
I would appreciate it if you could comment. |
I have committed a prototype version of GUI Testing. I have attached a sample flow to execute each action of the GUI Testing. |
Please remove the GUI testing from this Pull-Request. I have said multiple times the GUI testing work should not be part of the core flow testing plugin. Raise a separate pull-request with the GUI work in. By adding it to this PR you are blocking any progress being made to get the core of the flow testing framework moving forward. |
@HiroyasuNishiyama we again have the problem that this is a huge PR which has had months of development work happen without any feedback or discussion over important details. The bigger the PR, the more time I have to find to spend understanding it. That then takes longer for me to do the review, and you continue to work on it and make it even bigger (such as the 'action extension' apis and the gui testing work). I had hopped our regular status chats would help us do things in smaller iterations with better feedback and communication. Rather than end up with huge pieces of work that will take me days to understand properly. The fact I have to run a hitachi fork for Node-RED to even try out the plugin makes it harder still. So let us focus on one bit at a time. Following the initial design discussion, we identified some requirements on the core of Node-RED. The main one was adding custom edit panes to the node dialogs. In September last year we merged node-red/node-red#3130 - and I called out Flow Testing as being the reason for doing that work. I documented it here: node-red/designs#63 - it would appear I didn't do a good enough job to make you aware of it. I think that removes the need for the If there are other core changes required we need a PR to be raised so they can be discussed. |
If I have understood the implementation notes correctly, to run a test hooks are added to the message handling path. The existing flows are not stopped or modified otherwise. If you have a flow that starts with an MQTT In node, and you want to test different messages it could send, how does that work? Does it block any "real" messages the MQTT In node wants to send? Does it stop that node or disable it? This was one of the big open questions I had with how we would implement flow testing - how we manage the flows when we want to run a test. I had assumed we would need to stop the flows and restart them in a 'test' mode - but I wasn't sure how that would work and it doesn't look like that is the approach you have taken. |
I'm sorry. Removed GUI test from this PR. |
This PR is a very small prototype implementation to confirm the design direction of the flow tester.
This prototype implements the flow test configuration node and part of the flow test UI for each node.
The actions send and match are only partially implemented tentatively.
The following modifications to Node-RED are required for this plugin to work:
https://github.com/node-red-hitachi/node-red/tree/flow-test-support
[Sample Flow]