1215846 - Replace JS1.7 legacy generators with ES6 generators in toolkit/components/places

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: Replace legacy generators in toolkit/components/places

Please have a look at toolkit/components/places/tests/unit/test_history_notifications.js, with this patch the test fails, but as far as I can tell it never executed before.

Comment 2

[Marco Bonardo [:mak]]

(In reply to Tom Schuster [:evilpie] from comment #1)
> Please have a look at
> toolkit/components/places/tests/unit/test_history_notifications.js, with
> this patch the test fails, but as far as I can tell it never executed before.

Mixing up add_task and do_test_pending is not a great idea, they are 2 different ways to manage async tests (one new, one old) and I suspect they don't work well together at all, I think I may also have filed a bug in the past to explicitly fail tests mixing the two systems.
You should remove run_test completely and change the test to properly yield on a promise.

Why do you think it never executed? wouldn't it timeout due to do_test_pending if it didn't run to completion?

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8678472 [details] [diff] [review]
Replace legacy generators in toolkit/components/places

Review of attachment 8678472 [details] [diff] [review]:
-----------------------------------------------------------------

apart from what I said in previous comment, the other changes look good.

Comment 4

[Tom S. (please needinfo tschuster)]

(In reply to Marco Bonardo [::mak] from comment #2)
> You should remove run_test completely and change the test to properly yield
> on a promise.
I have zero ideas how that would look like.

> Why do you think it never executed? wouldn't it timeout due to
> do_test_pending if it didn't run to completion?
I think that, because it doesn't produce any output and it's basically only two lines.
 0:00.48 LOG: Thread-1 INFO (xpcshell/head.js) | test MAIN run_test pending (1)
 0:00.48 LOG: Thread-1 INFO (xpcshell/head.js) | test MAIN run_test finished (1)
Removing do_test_finished() from the test doesn't change anything. Adding do_check_true(false) before it doesn't either.

Comment 5

[Marco Bonardo [:mak]]

Sorry I forgot about this, let me try to fix that test locally.

Comment 6

[Marco Bonardo [:mak]]

Attachment: fix test_history_notifications.js

this should do

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8683619 [details] [diff] [review]
fix test_history_notifications.js

ehr I forgot a qref

Comment 8

[Marco Bonardo [:mak]]

Attachment: fix test_history_notifications.js

Comment 9

[Tom S. (please needinfo tschuster)]

Comment on attachment 8683626 [details] [diff] [review]
fix test_history_notifications.js

Review of attachment 8683626 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for helping me out!

::: toolkit/components/places/tests/unit/test_history_notifications.js
@@ -65,5 @@
>  
> -  // Make sure that the incorrectly opened service is closed before
> -  // we make another attempt. Otherwise, there will be a conflict between
> -  // the two services (and an assertion failure).
> -  yield shutdownPlaces();

So this is not needed anymore?

Comment 10

[Marco Bonardo [:mak]]

I didn't notice any issues without it, so I guess it's not needed anymore.

Comment 11

[Tom S. (please needinfo tschuster)]

Ups, seems like I forgot to update places/tests/browser/browser_colorAnalyzer.js properly.

Comment 12

[Tom S. (please needinfo tschuster)]

Attachment: fix browser_colorAnalyzer

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 8683860 [details] [diff] [review]
fix browser_colorAnalyzer

Review of attachment 8683860 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/browser/browser_colorAnalyzer.js
@@ +58,1 @@
>  // simple test - draw a red box in the center, make sure we get red back

nit: you added some newlines here and there that don't look useful off-hand

@@ -316,5 @@
> -  }, function(actual) {
> -    info("perfBigImage: " + ((new Date()) - t1) + "ms");
> -    nextStep();
> -  }, "", true);
> -});

why did you just remove the perf tests? I think someone added them to annotate them somewhere (this code is in Places but it shouldn't, so I can't give reasoning)... what about just commenting them out?

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66f266ae55b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b5c3d2306da

Comment 15

[Tom S. (please needinfo tschuster)]

The way the test was written doesn't work well with yield and promises. And performance tests in normal test suites are usually a bad idea anyway. If somebody wants the code back they can just look at the history.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66f266ae55b7
https://hg.mozilla.org/mozilla-central/rev/1b5c3d2306da