Demo-store command asking about running seeding scripts

chec / cli

A command line interface for using the Chec API

https://chec.io

BSD 3-Clause "New" or "Revised" License

9 stars 7 forks source link

Demo-store command asking about running seeding scripts #37

Closed rafaeelaudibert closed 4 years ago

rafaeelaudibert commented 4 years ago

Closes #28

I am having problems with the tests, as I've never used mocha and sinon before (I always use jest). I don't understand why the new test on the demo-store command test would fail, neither understand why tests would fail in other commands (such as it is now), as they don't call the code I've written. How can I mock the inquirer.prompt so that it correctly handles the login prompt or the seeding prompt?

I've tested the code per se, bringing it to the beginning of the demo-store command, and it works as intended:

robbieaverill commented 4 years ago

Yeah the test framework we're using here is unnecessarily verbose in my opinion, but it's what comes with oclif so we stuck with it. I'll take a look at your tests shortly, thanks for the contribution.

robbieaverill commented 4 years ago

Hi @rafaeelaudibert, I've had a bit of a play around with this PR and made the following changes:

Removed your test. I agree with you that the existing test suite we have in place is overly complex. It also has some bugs in it (unresolved async methods, memory leaks in event listeners, etc), and gives nothing useful when debugging the failure in the test you added. I'm happy to merge your PR without your test.
I changed the wording a little bit
Instead of returning early, we need to remove seed from the array of build scripts. Most of our demo stores define a list of "build scripts" in their .chec.json (example: https://github.com/chec/commercejs-nextjs-demo-store/blob/master/.chec.json#L3), and returning early means the other build scripts don't run. Most noticeably, the webpack dev server doesn't boot up if you return early. It does if you remove "seed" from the scripts array and continue.

robbieaverill commented 4 years ago

I believe the build failures are unrelated

robbieaverill commented 4 years ago

Hi again, I've removed the prompt to skip seeding. All we really need is the optional flag to skip it. The extra prompt was the cause of the failing tests, since prompts need to be mocked in the unit tests. Thanks again!

rafaeelaudibert commented 4 years ago

Hello @robbieaverill! Thanks for taking some time, and improving on the PR! Thank you also for the merge, hope it was helpful!

It is a pity the testing is not that great :(. Maybe we could try to test in another way, or are we really locked to oclif?