added suspending update renderer for task #136

bunysae · 2019-09-03T16:54:25Z

In order to fix issue np issue 79, these changes add an optional suspend-flag for every task.
The suspend-flag is later used, to suspend the listr-update-renderer.

lib/task.js

sindresorhus · 2019-09-14T08:05:30Z

lib/task.js

 	}

 	get subtasks() {
 		return this._subtasks;
 	}

+	shouldSuspendUpdateRenderer() {
+		try {


Why are you try/catching a boolean?

I catching it, because task.options can be undefined. Without catching it, it throws TypeError, if you don't supply any option.

sindresorhus · 2019-09-14T08:06:30Z

lib/task.js

@@ -43,12 +43,22 @@ class Task extends Subject {
 		this.title = task.title;
 		this.skip = task.skip || defaultSkipFn;
 		this.task = task.task;
+		this.taskOptions = task.options;


This is moot. You can already access it as this.task.options.

Sorry, but this.task can be just a function. task.options refers to input-variable task given in the constructor and not to this.task. We will need here a separate variable to store the options.

sindresorhus · 2019-09-14T08:06:46Z

The new option should be documented and tested.

bunysae · 2019-09-14T12:43:58Z

@sindresorhus:
Unit-tests are finished for the new feature.
Travis throw this error, because pull-request in listr-update-renderer isn't merged yet.

The diffs in the other files are coming from the command npx xo --fix.
It changed the position of Listr in the import-commands.

issue-comment: https://github.com/SamVerschueren/listr-update-renderer/pull/23/files/90210b0491209532723cbc6fe9f568c9d47fb357#r324416643

codecov-io · 2019-09-18T16:25:09Z

Codecov Report

Merging #136 into master will decrease coverage by 17.96%.
The diff coverage is 25%.

@@             Coverage Diff             @@
##           master     #136       +/-   ##
===========================================
- Coverage   96.31%   78.35%   -17.97%     
===========================================
  Files           7        7               
  Lines         190      194        +4     
===========================================
- Hits          183      152       -31     
- Misses          7       42       +35

Impacted Files	Coverage Δ
lib/task.js	`82.95% <25%> (-17.05%)`	⬇️
lib/listr-error.js	`33.33% <0%> (-66.67%)`	⬇️
lib/task-wrapper.js	`40% <0%> (-50%)`	⬇️
lib/state.js	`33.33% <0%> (-44.45%)`	⬇️
index.js	`92.72% <0%> (-7.28%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe40ce5...0327ff7. Read the comment docs.

bunysae · 2019-10-05T13:25:55Z

@sindresorhus:
In an second pull-request i repaired the failing tests.
Between the last successful build and now ava changed. and therefore a lot of the current tests are failing in the travis build.
I hope after merging the new pull-request, we can continue with merging this request.

bunysae · 2019-10-08T18:41:05Z

Anyone interested to review this pull-request and the affiliated pull-request 137?

lib/task.js

readme.md

itaisteinherz · 2019-12-20T14:21:54Z

test/concurrent.js

@@ -1,8 +1,8 @@
 import {serial as test} from 'ava';
 import delay from 'delay';
-import Listr from '..';


Why did you edit these files? They're out of the scope of this PR anyways, and the imports were fine the way they were.

The changes come from xo --fix command. I remove them in my next commit.

itaisteinherz · 2019-12-20T14:22:38Z

test/suspend.js

+		console.log(text);
+	}
+};
+logUpdateApi.main.clear = function () {


You should create a noop function (e.g. const noop = () => {};) and use it here and below.

I imported the log-update-module directly. This section now should be refactored.

itaisteinherz · 2019-12-20T14:24:34Z

test/suspend.js

+	mockery.enable({useCleanCache: true, warnOnUnregistered: false});
+});
+
+test('should suspend second task', async t => {


This doesn't test use-cases where suspendUpdateRenderer is actually needed. There should be a test(s) for that as well.

(See SamVerschueren/listr-update-renderer#23 (comment))

My intention was to create two task. The first task without the option suspendUpdateRenderer. So the update renderer should display status "task 1 pending" exactly one time. The second task with the option suspendUpdateRenderer. So the update renderer shouldn't display status "task 2 pending", because it is suspended. If the suspend-function doesn't work as expected, the update renderer displays "task 2 pending" more than once, because we have a timeout of 4s.
You could simply try it, when cloning my fork listr locally. With the current version of listr-update-renderer, the tests fail. With linking my fork of listr-update-renderer the tests pass.

What tests you thought of? Integration-tests, which ask for user input or tests, which mock listr-update-renderer?

What I thought of is a test case in which the suspension is actually required, meaning that the test won't pass if the task isn't suspended (and only passes if it is).
(Even if the mock isn't called, that doesn't really mean much. We want the test to verify correct behavior, not logic.)

I added now a custom renderer mock, which output is excatly verified. It only passes, if suspension is activated.
When the mock isn't called with the correct args, the test fails.

I'm sorry to keep bothering you, but this is still not what I meant in my comments above.
As I recall, the reason we need this in np is since one of the commands we call waits for the user to enter a password, which we need to receive before moving on to the next task.
This test, again, only verifies we correctly pass on the suspendUpdateRenderer option from here to the Listr renderer (let me know if I'm mistaken, though).
I think that the simplest way to test this would be to actually run a command that waits for user input, then verify we do receive it when suspendUpdateRenderer is set to true (and don't when it's set to false, as the task isn't suspended).

The problem isn't, that the task can't receive the user-input from stdin. The task actually receives the input from stdin independently of suspendUpdateRenderer. The problem is, that listr-update-renderer erases password-prompts on stdout. For the user it looks like the prompt never appears and the task is running infinite.
What we need to check is, that the task-output is not erased by the log-update-module, when a task with suspended renderer is running. I have done that in my actual version.

@itaisteinherz
Btw i added the testclass task-with-input, which proves, that the input from stdin is available for every task, no just for the tasks with suspended renderer.

readme.md

1. Refactoring test-classes and readme 2. play back import order

bunysae · 2020-04-30T14:24:06Z

@itaisteinherz or @LitoMore
Now the lint errors are fixed. Can you review it again?

LitoMore · 2020-05-25T14:45:23Z

@bunysae Thank you so much! I will review your PR in few days.

sindresorhus · 2020-07-15T18:45:16Z

@LitoMore @SamVerschueren Bump :)

Bunyanuch Saengnet and others added 2 commits September 2, 2019 17:43

added suspending update renderer for task

5a1272a

fixed clinton && xo && nyc ava errors

470a8b8

bunysae mentioned this pull request Sep 14, 2019

Support GPG password prompt sindresorhus/np#448

Closed

sindresorhus reviewed Sep 14, 2019

View reviewed changes

lib/task.js Outdated Show resolved Hide resolved

sindresorhus reviewed Sep 14, 2019

View reviewed changes

bunysae added 3 commits September 17, 2019 19:12

unit-test for suspending update-rendering added

c0317c7

documentation for suspending-feature

e440c18

Moved feature from listr-update-renderer to listr according to

7a74f5d

issue-comment: https://github.com/SamVerschueren/listr-update-renderer/pull/23/files/90210b0491209532723cbc6fe9f568c9d47fb357#r324416643

bunysae added 2 commits September 18, 2019 16:37

fixed issue in unit-test

4b2c801

fixed xo-error

f2e2750

itaisteinherz reviewed Dec 20, 2019

View reviewed changes

lib/task.js Outdated Show resolved Hide resolved

itaisteinherz reviewed Dec 20, 2019

View reviewed changes

readme.md Outdated Show resolved Hide resolved

itaisteinherz reviewed Dec 20, 2019

View reviewed changes

readme.md Outdated Show resolved Hide resolved

itaisteinherz reviewed Dec 20, 2019

View reviewed changes

readme.md Outdated Show resolved Hide resolved

Added suspending update renderer for task:

8455f63

1. Refactoring test-classes and readme 2. play back import order

bunysae force-pushed the master branch from 9f1dadf to 6eeb4c4 Compare February 18, 2020 18:26

bunysae added 5 commits February 18, 2020 18:27

Refactoring test-module

6eeb4c4

Removed mockery dependency

47fb5a2

Refactoring suspend-testclass

1cf0f10

Add testclass for user-input-verification

0327ff7

Integrate lint fixes

607a7f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added suspending update renderer for task #136

added suspending update renderer for task #136

bunysae commented Sep 3, 2019

sindresorhus Sep 14, 2019

bunysae Sep 14, 2019 •

edited

Loading

sindresorhus Sep 14, 2019

bunysae Sep 14, 2019 •

edited

Loading

sindresorhus commented Sep 14, 2019

bunysae commented Sep 14, 2019 •

edited

Loading

codecov-io commented Sep 18, 2019 •

edited

Loading

bunysae commented Oct 5, 2019 •

edited

Loading

bunysae commented Oct 8, 2019

itaisteinherz Dec 20, 2019

bunysae Jan 4, 2020

itaisteinherz Dec 20, 2019

bunysae Jan 4, 2020

itaisteinherz Dec 20, 2019

itaisteinherz Dec 20, 2019

bunysae Jan 4, 2020

itaisteinherz Feb 18, 2020 •

edited

Loading

bunysae Feb 18, 2020

itaisteinherz Feb 27, 2020

bunysae Mar 6, 2020 •

edited

Loading

bunysae Mar 28, 2020

bunysae commented Apr 30, 2020 •

edited

Loading

LitoMore commented May 25, 2020 •

edited

Loading

sindresorhus commented Jul 15, 2020

added suspending update renderer for task #136

Are you sure you want to change the base?

added suspending update renderer for task #136

Conversation

bunysae commented Sep 3, 2019

Choose a reason for hiding this comment

bunysae Sep 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bunysae Sep 14, 2019 • edited Loading

Choose a reason for hiding this comment

sindresorhus commented Sep 14, 2019

bunysae commented Sep 14, 2019 • edited Loading

codecov-io commented Sep 18, 2019 • edited Loading

Codecov Report

bunysae commented Oct 5, 2019 • edited Loading

bunysae commented Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaisteinherz Feb 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bunysae Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bunysae commented Apr 30, 2020 • edited Loading

LitoMore commented May 25, 2020 • edited Loading

sindresorhus commented Jul 15, 2020

bunysae Sep 14, 2019 •

edited

Loading

bunysae Sep 14, 2019 •

edited

Loading

bunysae commented Sep 14, 2019 •

edited

Loading

codecov-io commented Sep 18, 2019 •

edited

Loading

bunysae commented Oct 5, 2019 •

edited

Loading

itaisteinherz Feb 18, 2020 •

edited

Loading

bunysae Mar 6, 2020 •

edited

Loading

bunysae commented Apr 30, 2020 •

edited

Loading

LitoMore commented May 25, 2020 •

edited

Loading