1068011 - TPS broken due to 'let' changes in bug 1001090

Status: RESOLVED FIXED

(Testing Graveyard :: TPS, defect, P1)

Description

[Cosmin Malutan, [:cosmin-malutan]]

TPS extension doesn't works anymore with the latest change from bug 1001090.

This requires to check all instances where we use let to make sure we don't declare the same variable twice. 


04:47:20 Running test test_sync.js
04:47:20 
04:47:20 JavaScript error: resource://tps/tps.jsm, line 823: TypeError: redeclaration of variable cb
04:51:21 
04:51:21 TEST-UNEXPECTED-FAIL | test_sync.js | test did not complete
04:51:21 
04:51:21 	phase1: unknown

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please note that this bug also needs an upgrade of Mozmill to version 2.0.8!

Also this is a critical regression and should be marked as such, and the causing bug needs to be in the blocking list.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

This patch changes all 'let' keywords to 'var' in TPS and it upgrades the mozmill extension to 2.0.8
I ran a complete testrun with the latest nightly and it ran perfectly.

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.1

I added DONTBUILD tag.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8490602 [details] [diff] [review]
patch v1.0

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

I don't see the reason why we have to replace any let statement with var. This is something I already mentioned yesterday to Andrei for the Mozmill fix. Fix the real issue but do not do massive replacements of code. Also the Mozmill update should be its own attachment. Don't squeeze all together into a single patch.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: TPS changes v1 [checked-in]

This is the change needed in TPS

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.2 MOZMILL

Here is the patch to update mozmill to 2.0.8

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8490640 [details] [diff] [review]
TPS changes v1 [checked-in]

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

Looks trivial enough.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8490641 [details] [diff] [review]
patch v1.2 MOZMILL

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

Something what really worries me on this patch is the fact that changes, which were done a long time ago, have not been integrated with the last version bump of the Mozmill extension for TPS some weeks ago! Also again, the changes as landed for Mozmill 2.0.8 are totally not part of this patch!

Cosmin, please tell us how you actually updated the mozmill extension. The current method is not going to work.

::: services/sync/tps/extensions/mozmill/chrome.manifest
@@ -1,1 @@
> -resource mozmill resource/

Not sure when this came in, but it was definitely not between Mozmill 2.0.6 and 2.0.8. Looks like the last bump of Mozmill was not correctly done some weeks ago.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8490640 [details] [diff] [review]
TPS changes v1 [checked-in]

I landed this patch directly on mozilla-central. Not sure if it will make the next Nightly builds. We will see.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> Something what really worries me on this patch is the fact that changes,
> which were done a long time ago, have not been integrated with the last
> version bump of the Mozmill extension for TPS some weeks ago! Also again,
> the changes as landed for Mozmill 2.0.8 are totally not part of this patch!
> 
> Cosmin, please tell us how you actually updated the mozmill extension. The
> current method is not going to work.
This is exactly how it was made in bug 982610.
So here is how I done this:
- I cloned mozmill from github, I checked out branch 2.0.8
- I copied the files form  /mozmill/mozmill/extension into tps/extensions/mozmill
- I removed components and content directories as they are not needed for the extension to work but for the mozmill python side or building.
Please keep in mind that in TPS we use the extension only, not the jsBridge and not the python(server side).

> ::: services/sync/tps/extensions/mozmill/chrome.manifest
> @@ -1,1 @@
> > -resource mozmill resource/
> 
> Not sure when this came in, but it was definitely not between Mozmill 2.0.6
> and 2.0.8. Looks like the last bump of Mozmill was not correctly done some
> weeks ago.
It was correctly done, reviewed, tested and it didn't failed in the past 4 months we lived with it. Again it looks strange and it might not include all the changes because we don't use the whole code.
I've got nothing to change from my side, if there is something wrong please point it to me and I'll change it.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8490641 [details] [diff] [review]
patch v1.2 MOZMILL

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

(In reply to Cosmin Malutan from comment #10)
> This is exactly how it was made in bug 982610.

This cannot be true because as you can see lines of code are getting changed in your patch, which didn't exist in the Mozmill repository for a longer time.

> - I removed components and content directories as they are not needed for
> the extension to work but for the mozmill python side or building.

Something you also had to do is the modification of some files. So most likely you missed to remove those lines the last time when editing the appropriate files.

> Please keep in mind that in TPS we use the extension only, not the jsBridge
> and not the python(server side).

I know that, and this is not the problem here.

> It was correctly done, reviewed, tested and it didn't failed in the past 4
> months we lived with it. Again it looks strange and it might not include all
> the changes because we don't use the whole code.

Given that those were only comments nothing affected the functionality of Mozmill itself. It's just a change which wasn't present before. So this raised confusion.

> I've got nothing to change from my side, if there is something wrong please
> point it to me and I'll change it.

I manually checked all the files updated with this patch and it seems to be fine. I will get it landed in a bit.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Btw, the mozmill patch does not contain a valid commit message. I got this updated before landing it:

https://hg.mozilla.org/mozilla-central/rev/5afba113494b

We are not going to update older branches, given that we want to extract the tps framework soon from the tree.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 8490640 [details] [diff] [review]
> TPS changes v1 [checked-in]
> 
> I landed this patch directly on mozilla-central. Not sure if it will make
> the next Nightly builds. We will see.

I missed to give the changeset for that landing:

https://hg.mozilla.org/mozilla-central/rev/641d450532be