Missing test run script should not fail np when using Yarn

[sindresorhus]

When using npm it just exists with code 0 when the test script is missing, but when using Yarn it exists with code 1. I've partially fixed it in acbaece, it passes the step, but it still shows an error message after exiting.

 ✔ Prerequisite check
 ✔ Git
 ✔ Running tests using Yarn
 ↓ Bumping version using Yarn [skipped]
   → Public package: version will be bumped using yarn publish.
 ✔ Publishing package using Yarn
 ✔ Pushing tags

 sindre-playground 0.23.12 published 🎉
Error: Command failed: yarn test


    at makeError (/Users/sindresorhus/dev/oss/np/node_modules/execa/index.js:172:9)
    at Promise.all.then.arr (/Users/sindresorhus/dev/oss/np/node_modules/execa/index.js:277:16)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
Error: Command failed: yarn test


    at makeError (/Users/sindresorhus/dev/oss/np/node_modules/execa/index.js:172:9)
    at Promise.all.then.arr (/Users/sindresorhus/dev/oss/np/node_modules/execa/index.js:277:16)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

dflupu earned $80.00 by resolving this issue!

• Checkout the Issuehunt explorer to discover more funded issues.
• Need some help from other developers? Add your repositories on Issuehunt to raise funds.

[sindresorhus]

I have a feeling this is caused by

np/index.js

Lines 17 to 25 in cec8e00

	const exec = (cmd, args) => {
	// Use `Observable` support if merged https://github.com/sindresorhus/execa/pull/26
	const cp = execa(cmd, args);
	return merge(
	streamToObservable(cp.stdout.pipe(split()), {await: cp}),
	streamToObservable(cp.stderr.pipe(split()), {await: cp})
	).pipe(filter(Boolean));
	};

or rather stream-to-observable, but I haven't looked closely into it.

[IssueHuntBot]

@issuehuntfest has funded $80.00 to this issue. See it on IssueHunt

[Norris1z]

@sindresorhus running echo $? after yarn test with no test run script returns 1 whereas it returns 0 for npm. Don't you think this is caused by yarn itself?

[sindresorhus]

@Norris1z Yup, but another part of the problem that needs to be fixed is that it only shows the error on exit, and doesn't affect the np exit code, instead of throwing it right away, so something about the error handling and Observables are wrong.

[Norris1z]

@sindresorhus you're right. It is a problem with the Observables.

title: "Running tests using Yarn",
enabled: () => opts.yarn === true,
task: () =>
  execa.stdout("yarn", ["test"]).catch(error => {
    if (!error.message.includes('Command "test" not found')) {
        throw new Error(error);
   }
})

This method works great. Should i submit a pr for this or we should still debug the Observables?

[sindresorhus]

@Norris1z We need to use Observables so the child process output is streamed into the task output. With your change the test output will only be shown when it's done.

we should still debug the Observables?

Yes

[Norris1z]

@sindresorhus i found out that somehow after the failed yarn test command, two empty commands are sent to execa which produces { code: 1, signal: null, stdout: '', stderr: '' } and because both stdout and stderr are empty it slips through the observables somehow. I posted an issue on the execa repo to clarify if { code: 1, signal: null, stdout: '', stderr: '' } was a valid or if we could perform an extra check on execa to catch errors with both stdout and stderr empty.

[IssueHuntBot]

@dflupu has submitted a pull request. See it on IssueHunt

[IssueHuntBot]

@sindresorhus has rewarded $72.00 to @dflupu. See it on IssueHunt

• 💰 Total deposit: $80.00
• 🎉 Repository reward(0%): $0.00
• 🔧 Service fee(10%): $8.00