fix: empty spaces uncorrectly detected as positional argument #626
Conversation
sinedied
requested review from
manekinekko,
sgollapudi77 and
sulabh-msft
as code owners
| it("should not interpret empty spaces as a positional argument", async () => { | ||
| const execSyncMock = jest.requireMock("child_process").execSync; | ||
| mockFs(); | ||
| await run(["node", "swa", "build", " ", "--app-build-command", "npm run something", " "]); |
There was a problem hiding this comment.
Great thanks! @sinedied for adding the test for swa build..
Just to confirm if these tests cover the changes we did in deploy.ts/start.ts if not will it make sense for us to add separate tests for those scenarios ?
There was a problem hiding this comment.
The added logic is the same for build/start/deploy, so it should validate these changes, but not cover it in case of refactor of start/deploy.
Currently there's no tests (at all) for the cli commands deploy/start/login, hence why I reused the mocks made for the build command tests. Adding minimal tests for these commands would be useful, though not the scope of this PR.
|
@sgollapudi77 / @rupareddy5-21 Can you also please review the PR ? |
|
Hmmm, seems in some Node environments jest fails to restore properly the "child_process" mocks. making the tests fail. I had a hard time finding a workaround already locally, I'll try to use another approach then and refactor the code a bit. |
|
Tests are passing now. I refactored the cli commands architecture a bit to split the registering (=doc and options parsing) and the execution parts. Testing individual commands and their components should be easier now. |
|
@sinedied looks like there is one merge conflict. Can you please resolve the merge conflict ? |
sinedied
force-pushed
the
fix-empty-arg
branch
from
8b9e061 to
f93b9ae
Compare
|
@sulabh-msft I've updated the branch, no more conflict :) |
Empty spaces are currently misdetected as positional arguments, for example
swa deploy --output-location distwill incorrectly interpret the space as a positional argument and print an error because it conflicts with theoutput-locationoption.This PR fixes that.