Enhancement: Supporting deep comparison of tasks with reference value in custom properties #9647

magician-margatroid · 2021-06-28T03:18:36Z

What it does

Issue link: in short, tasks with same content but different reference value as custom properties are treated as different tasks, e.g., [], {prop: 'value'}. This can lead to duplicate execution of same task.

This PR includes 2 commits:

Add several unit test cases for task-definition-registry.ts to ensure the logic of distinguishing.
Change shallow comparison into deep comparison of properties in compareTasks method.

How to test

Unit test:
- I've added some cases in the 1st commit.
- Before the changes in 2nd commit, tests for @theia/task will ends up with 33 passing and 2 failing.
- After the changes in 2nd commit, tests for @theia/task will ends up with 35 passing and 0 failing.
Manually integration test:
- Just follow the exact steps in the issue linked above.
- Before changes, the task can be run for several time no need former execution ends.
- After changes, "Task is already running" will be prompted for user if former execution doesn't end. (Which is also in consistency with VSCode.)

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

These cases includes the conditions when tasks with reference value as custom properties. In tests with @thiea/task, 33 passing and 2 failing is expected. Signed-off-by: Victor Li <[email protected]>

…operties deeply. All the unit-test cases for @thiea/task is expected to pass. (35 passing, 0 failing) Signed-off-by: Victor Li <[email protected]>

vince-fugnitto · 2021-06-28T12:05:26Z

packages/task/src/browser/task-definition-registry.spec.ts

@@ -15,6 +15,7 @@
 ********************************************************************************/

 import { expect } from 'chai';
+import { foobarTaskFixture } from '../node/test/test-helper';


@magician-margatroid browser (frontend) should not import anything from node (backend), even if it is simply for test purposes: code organization. Given that the foobarTaskFixture is simply for test purposes, you can include the contents directly in the task-definition-registry.spec.ts file.

Thank you very much @vince-fugnitto . Excuse me and I'll improve it.

* Moving foobarTaskFixture into task-definition-registry.spec.ts. Signed-off-by: Victor Li <[email protected]>

magician-margatroid · 2021-06-28T13:26:55Z

Hi @vince-fugnitto , when you have the time, could you pls help to have a look at the build check result?

As it indicates, it seems the Build / ubuntu-18.04, Node.js v12.x (pull_request) failed after 14m — ubuntu-18.04, Node.js v12.x.

I found root ERROR [nsfw-watcher: 9379] NSFW service error on "/home/runner/work/theia/theia/examples/browser": [Error: Service shutdown unexpectedly] and several other errors in the log, but still not very sure about the root cause.

Personally I think this new commit doesn't have side effects, and also executed unit-tests locally with all-green.

magician-margatroid · 2021-06-28T14:15:59Z

Hi @vince-fugnitto , when you have the time, could you pls help to have a look at the build check result?

As it indicates, it seems the Build / ubuntu-18.04, Node.js v12.x (pull_request) failed after 14m — ubuntu-18.04, Node.js v12.x.

I found root ERROR [nsfw-watcher: 9379] NSFW service error on "/home/runner/work/theia/theia/examples/browser": [Error: Service shutdown unexpectedly] and several other errors in the log, but still not very sure about the root cause.

Personally I think this new commit doesn't have side effects, and also executed unit-tests locally with all-green.

Thanks. This time all that succeeded.

vince-fugnitto · 2021-06-28T14:46:08Z

packages/task/src/browser/task-definition-registry.spec.ts

+        reveal: RevealKind.Always,
+        showReuseMessage: true,
+    };
+    const foobarTaskFixture = {


minor: what does a fixture represent, it is the first time I heard of this terminology for tasks.

Nice point. Well, frankly I didn't aim to use it exclusively for tasks, but I mean to use "test fixtures" here. In my understanding fixture can be something stable across runs of tasks. E.g., a file loaded with sample data or a script setting up stable env for task.

Perhaps here I need to change it into mock or fake.

vince-fugnitto · 2021-06-28T14:47:22Z

packages/task/src/browser/task-definition-registry.spec.ts

+                foobarTaskFixture.conf('id_1', 'type_1'),
+                foobarTaskFixture.conf('id_2', 'type_2'),
+            );
+            expect(res).to.be.not.ok;


minor: rather than okayness, we can simply do truthy checks.

expect(res).to.be.true;

or

expect(res).equal(true);

Good suggestion. I'll change it in this way.

vince-fugnitto · 2021-06-28T14:52:10Z

packages/task/src/browser/task-definition-registry.spec.ts

+        });
+
+        it('should return true if given 2 same task configurations with empty arrays (different by reference) as custom property', () => {
+            const res = registry.compareTasks(


I see the use of res a lot in the tests, what does it represnt? Perhaps a clearer name should be used?

Nice suggestion. I agree and I meant "result" here. I'll change them.

* Change okayness into truthy checks. Signed-off-by: Victor Li <[email protected]>

alvsan09

I have tested this change and it works as expected 👍

The code looks good to me as well!
Thanks @magician-margatroid !

magician-margatroid · 2021-07-16T01:05:36Z

I have tested this change and it works as expected

The code looks good to me as well!
Thanks @magician-margatroid !

Ah, good to know it, @alvsan09 thx for your testing. It's my pleasure.

vince-fugnitto

The changes work well for me, I verified with the described steps in #9646 👍
Thank you for the contribution, and looking forward to many more in the future!

RomanNikitenko · 2021-07-16T14:08:28Z

I tried to test according to the steps to reproduce secion.
It's strange, but I don't see the custombuildscript: 32 watch incremental task

For the master I do see it

Also, the order is different.
Could someone confirm that the problem is on my side only...
thanks in advance!

p/s
I installed the extension as vsix
I used multi-root workspace for testing
I'm on macos

RomanNikitenko · 2021-07-16T14:49:36Z

I found what's the problem...
It looks like I selected Never scan the task output at running the task.
Then the task was converted to configured with another label.

cust_tasks_config.mp4

RomanNikitenko

Tested according to the steps to reproduce section of the issue, it works well for me.
I think it makes sense to squash the changes in 1 or 2 commits before merging.

vince-fugnitto · 2021-07-16T16:04:42Z

I used the new squash and merge option to merge the commit 👍

magician-margatroid · 2021-07-19T01:25:14Z

Hi everyone, thanks a lot for all your work. It's my pleasure and appreciate your help and analysis all the way.

… in custom properties (eclipse-theia#9647) The commit adds deep comparison of tasks, including tests: - use `JSONExt.deepEqual` from `@phosphor/coreutils` to compare custom properties deeply. - add unit-test cases for `compareTasks` in `task-definition-registry.ts`: these cases includes the conditions when tasks with reference value as custom properties. Signed-off-by: Victor Li <[email protected]>

magician-margatroid added 2 commits June 28, 2021 04:51

* Add unit-test cases for compareTasks in task-definition-registry.ts.

cf42e65

These cases includes the conditions when tasks with reference value as custom properties. In tests with @thiea/task, 33 passing and 2 failing is expected. Signed-off-by: Victor Li <[email protected]>

* Use JSONExt.deepEqual from @phosphor/coreutils to compare custom pr…

a204609

…operties deeply. All the unit-test cases for @thiea/task is expected to pass. (35 passing, 0 failing) Signed-off-by: Victor Li <[email protected]>

vince-fugnitto reviewed Jun 28, 2021

View reviewed changes

vince-fugnitto added the tasks issues related to the task system label Jun 28, 2021

* Removing node/test/test-helper.ts added in the 1st commit.

75c573e

* Moving foobarTaskFixture into task-definition-registry.spec.ts. Signed-off-by: Victor Li <[email protected]>

vince-fugnitto reviewed Jun 28, 2021

View reviewed changes

* Rename variables in unit test cases.

2c8d0b7

* Change okayness into truthy checks. Signed-off-by: Victor Li <[email protected]>

vince-fugnitto requested review from RomanNikitenko and tsmaeder June 28, 2021 15:29

alvsan09 reviewed Jul 15, 2021

View reviewed changes

vince-fugnitto approved these changes Jul 16, 2021

View reviewed changes

RomanNikitenko approved these changes Jul 16, 2021

View reviewed changes

vince-fugnitto merged commit d5c9fb7 into eclipse-theia:master Jul 16, 2021

vince-fugnitto added this to the 1.16.0 milestone Jul 29, 2021

vince-fugnitto mentioned this pull request Jul 30, 2021

TaskService.runTask(...) not safe for parallel Execution #9806

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: Supporting deep comparison of tasks with reference value in custom properties #9647

Enhancement: Supporting deep comparison of tasks with reference value in custom properties #9647

magician-margatroid commented Jun 28, 2021 •

edited by vince-fugnitto

Loading

vince-fugnitto Jun 28, 2021

magician-margatroid Jun 28, 2021

magician-margatroid commented Jun 28, 2021

magician-margatroid commented Jun 28, 2021

vince-fugnitto Jun 28, 2021

magician-margatroid Jun 28, 2021

vince-fugnitto Jun 28, 2021

magician-margatroid Jun 28, 2021

vince-fugnitto Jun 28, 2021

magician-margatroid Jun 28, 2021

alvsan09 left a comment

magician-margatroid commented Jul 16, 2021

vince-fugnitto left a comment

RomanNikitenko commented Jul 16, 2021 •

edited

Loading

RomanNikitenko commented Jul 16, 2021

RomanNikitenko left a comment

vince-fugnitto commented Jul 16, 2021

magician-margatroid commented Jul 19, 2021

Enhancement: Supporting deep comparison of tasks with reference value in custom properties #9647

Enhancement: Supporting deep comparison of tasks with reference value in custom properties #9647

Conversation

magician-margatroid commented Jun 28, 2021 • edited by vince-fugnitto Loading

What it does

How to test

Review checklist

Reminder for reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magician-margatroid commented Jun 28, 2021

magician-margatroid commented Jun 28, 2021

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

alvsan09 left a comment

Choose a reason for hiding this comment

magician-margatroid commented Jul 16, 2021

vince-fugnitto left a comment

Choose a reason for hiding this comment

RomanNikitenko commented Jul 16, 2021 • edited Loading

RomanNikitenko commented Jul 16, 2021

RomanNikitenko left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Jul 16, 2021

magician-margatroid commented Jul 19, 2021

magician-margatroid commented Jun 28, 2021 •

edited by vince-fugnitto

Loading

RomanNikitenko commented Jul 16, 2021 •

edited

Loading