geostyler / geostyler-cli

BSD 2-Clause "Simplified" License
24 stars 9 forks source link

Add workflow and command line tests #343

Closed geographika closed 1 year ago

geographika commented 1 year ago

Currently testing on all branches but can revert to master following a review.

This adds a GHA to build the client and run a few test examples from the command line. It then does a simple check to see that the file was created.

KaiVolland commented 1 year ago

@marcjansen as we talked about it. You want to have a look at this. Any thougths?

jansule commented 1 year ago

Do you think we can find a way to put the tests into a script so it is also possible to run them locally?

marcjansen commented 1 year ago

This is a great addition, I think.

I was thinking of rcreating the complete CLI, e.g. with https://github.com/sindresorhus/node-cli-boilerplate/tree/main (That one would be without typescript), but I have noit had the chance to really start this.

This would probably be even saner, bit this is a nice addition nonetheless.

geographika commented 1 year ago

Do you think we can find a way to put the tests into a script so it is also possible to run them locally?

@jansule - I've rewritten these tests as a script and added to package.json. It runs on Windows locally for me and on GHA, I'm not sure why the output goes to stderr rather than stdout though.

jansule commented 1 year ago

Do you think we can find a way to put the tests into a script so it is also possible to run them locally?

@jansule - I've rewritten these tests as a script and added to package.json. It runs on Windows locally for me and on GHA, I'm not sure why the output goes to stderr rather than stdout though.

That is weird, indeed. I just ran following code snippet from https://nodejs.org/api/child_process.html#child_processspawncommand-args-options and this seems to print to stdout:

const { spawn } = require('node:child_process');
const ls = spawn('ls', ['-lh', '/usr']);

ls.stdout.on('data', (data) => {
  console.log(`stdout: ${data}`);
});

ls.stderr.on('data', (data) => {
  console.error(`stderr: ${data}`);
});

ls.on('close', (code) => {
  console.log(`child process exited with code ${code}`);
});
geographika commented 1 year ago

It looks like this is by design (or at least default) when using the ora package used for output:

https://github.com/sindresorhus/ora/blob/476935f318868265303d148992fc268639a0d573/index.d.ts#L80-L87

        /**
        Stream to write the output.

        You could for example set this to `process.stdout` instead.

        @default process.stderr
        */
        readonly stream?: NodeJS.WritableStream;
jansule commented 1 year ago

According to https://nodejs.org/api/child_process.html#optionsstdio the default stdio configuration is ['pipe', 'pipe', 'pipe']. So I would expect the output of the child process piped to the parent process.

geographika commented 1 year ago

Thanks for the review @KaiVolland. Once merged the tests will fail due to #344 but I plan to create a follow-up pull request with a fix for this and a test for multi-file output.

dnlkoch commented 1 year ago

:tada: This PR is included in version 3.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: