fix: prevent expect step to fail silently

[Maestro31]

This PR to prevent scenario.run with expect step to fail silently. Related to #3

Currently, I assume that only the expect step should raise an error but I can imagine that the input step will fail silently under the same conditions.

We may have to look at a more global solution, but I feel like that would require a lot of restructuring, including having the steps as separate classes with their own behavior.

I'm open to discussion (en français aussi :D)

Thanks for the review

[tony-go]

Hi @Maestro31 👋

Thank you so much for your interest in clix ^^

Maybe could we catch this error when we spawn the process:

clix/src/scenario.js

Line 214 in dcd6082

async #spawnCommand() {

We could handle the error like this:

proc.on('error', (err) => { // todo });

WDYT?

[Maestro31]

@tony-go, i'm not sure but it seems to me that with your proposal, in case we catch the error and use it to update #buffer.err, it doesn't change anything to the current behavior because the error is well added to #buffer.err in any case. And in the case where we raise the error directly, we stop the execution of the test even before checking that the "step matcher" is waiting for a possible error.

The simplest way seems to me to let the "step matcher" choose the behavior. Today, it's in the body of a switch case, but it could be classes with a dedicated method that takes as input the data of the current step and returns a comparison result, or raise an error if not expected.

[tony-go]

Thanks for sharing your thoughts.

IMO we should stop the .run as soon as we can. That being said, we should catch only a certain type of error avoiding catching expected errors. We want to catch errors related to child_process start, like a wrong command (with a typo).

We probably have to investigate aiming to find if we could differentiate kinds of errors.

I'd like to keep the usage of #buffer.err for "normal" errors (expected ones).

[Maestro31]

I spent some time today experimenting with a specific branch: https://github.com/Maestro31/clix/tree/refactor-code

I went with the principle of reversing the loop to start from the events received from the child process rather than the scenario steps.

By making these changes, the error is effectively lifted as soon as a new data is received as you suggest.

By the way, I took out the responsibility of the scenario construction in a scenario builder and extracted the matchers logic in separate classes.

I also removed the _compare method tests which were not relevant anymore after these changes and we can easily add matchers specific tests now (todo).

You can also add additional matchers quite easily. Potentially even provide an API to add custom matchers.

I would be interested in your feedback in any case!

[tony-go]

Awesome 🙌

I went with the principle of reversing the loop to start from the events received from the child process rather than the scenario steps.

I was hesitating between both when I started 😅.

Thanks for all these efforts. The implementation is quite impressive 💪

But I still had a concern, we started from a request (handle unknown command) and we are now with a full new implementation. I think that it's not the way I'd like to see this project evolving.
I'd like to see the design evolving piece by piece, each time it's necessary. For example in our case, my idea was to isolote the Process as you did (💡). So for the purpose of this task, it makes a lot of sense to use this part of your implementation and add proper tests to validate the case (and unit test for this Process class)

WDYT?

---

For the rest, we could keep it as a reference for the future, even If I'm not sure about the scenario / scenario-builder abstract split. Maybe we could discuss this point further.

[Maestro31]

Thanks for the feedback ! 🙏

Yes, I couldn't imagine opening a PR with all the changes 😅. I'm going to work on the necessary process changes.

For the scenario builder, let's say it allows to separate the responsibility of validation and scenario building.
Scenario being responsible only for running the command and orchestrating the process.

Another advantage is that it avoids Scenario to know the implementation and the way to instantiate the matchers. So we can refactor these parts independently without touching Scenario.

[Maestro31]

@tony-go , I just made the changes. In order to be able to start from the received data instead of the steps, I had to modify the api of input because it also allows to avoid the process at the expect before the input. Not receiving data is blocking with this paradigm shift.

We can discuss this change of api for input matcher, we could consider keeping the construction of the step in two time, or keep one with a different api. As long as we keep one step only at the time of the process it's ok.

edit: it is not necessary to unit test the process class because the coverage is already reached by the other tests. It is just a refactoring without changing the behaviors.

[tony-go]

Really happy to see your new commits this morning. 👍

Before making a full a review of it, I'd like to highlight a bunch of stuff:

• I'm not a fan of the new .input API that doesn't express what you trying to do now. Moreover, it breaks our previous API and I'd like to stick to it ATM.

  • I had to modify the api of input because it also allows to avoid the process at the expect before the input.

  • -> I think that an implementation detail should not influence the API itself. WDYT?
• You should rebase/merge the v0.0.2 branch (we'll use a version branch, sorry for the lack of contributing.md 🙈)
• Test doesn't pass on the CI.

edit: it is not necessary to unit test the process class because the coverage is already reached by the other tests. It is just refactoring without changing the behaviors.

I have a kind of different opinion on that. We should have unit tests as having functional tests is not a reason for not having unit tests. But we could add them after if functional cover the overall atm.

We can discuss this change of api for input matcher

We'll try to deliver a first version that sticks to our promise (the README), then we could think about having customer matchers (if we found proper cases for it)

Again, amazing work, we'll make it 💪

[tony-go]

(I'll try to rebase to v0.0.2)

[tony-go]

I succeed to rebase 🎉

Integrating on stream instead of user steps (this.steps) increases drastically performances:

Running functional tests on my machine

Before	After
~ 6 secs	~ 2 secs 🎉

Honestly, it was my main concern with clix ahah ^^ Even If I' was planning to tackle it further, you rock!

---

What is remaining ?

• The CI still doesn't pass on the case you add (in run.test)

• (cp) I'm not a fan of the new .input API that doesn't express what you trying to do now. Moreover, it breaks our previous API and I'd like to stick to it ATM.

    I had to modify the api of input because it also allows to avoid the process at the expect before the input.
  

  -> I think that an implementation detail should not influence the API itself. WDYT?

• The example folder doesn't pass anymore also

[tony-go]

I'm currently tweaking the code. Please don't push new updates for the moment. 🙏

[tony-go]

I'll make a few more iterations and we should be good.

[Maestro31]

It sounds good for me too !