Closed Bug 1466899 Opened 6 years ago Closed 6 years ago

TPS - Tabs: allow profile property be optional or at least match variable profile

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: isabel_rios, Assigned: markh)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Trying to implement an integration test which opens a tab on firefox-ios and checks that it is open on desktop is giving an error. We are facing an issue with the profile match.
The test fails in the assertion because the profile is different depending on the environment the test runs.

Would it be possible to make this property optional or at least allow a fuzzy match like "Fennec on *"?

This is how we are defining the test_tabs.js:
EnableEngines(["tabs"]);

var phases = { "phase1": "profile1" };

// expected tabs state
var tabs1 = [
    { uri: "www.mozilla.org",
      profile: "profile1"
    }
];

// sync and verify tabs
Phase("phase1", [
  [Sync],
  [Tabs.verify, tabs1]
]);

But the profile changes for each environment so, if we change the profile to the environment's owner profile, then the test passes.

If changing the property to optional or allowing a fuzzy match is not allowed, is there any workaround we can follow to get the test working?

Thanks
From a quick look, the "profile" entry in a tabs array is probably better named something like "clientName", so yeah, using a regex or similar would probably work fine. However, it seems like it would be more valuable if you could ensure the device name for the iOS device was fully under your control for a future where multiple iOS or Android devices could be involved in a single test. If you did that, you probably wouldn't need a change here as you could just make the iOS device be named "profile1".
(In reply to Mark Hammond [:markh] from comment #1)
> From a quick look, the "profile" entry in a tabs array is probably better
> named something like "clientName", so yeah, using a regex or similar would
> probably work fine. However, it seems like it would be more valuable if you
> could ensure the device name for the iOS device was fully under your control
> for a future where multiple iOS or Android devices could be involved in a
> single test. If you did that, you probably wouldn't need a change here as
> you could just make the iOS device be named "profile1".

Thanks Mark, you make a great point. Isabel is looking into setting the iOS client name via the UI to see if that works, though I believe bug 1466940 will make testing this a little easier. If this does work, we can look into alternative ways of setting the client name to avoid uneccesary UI interactions.
Depends on: 1467585
Attached patch tabs.patch (obsolete) — Splinter Review
I think that the following patch should work - it should check the correct client name rather than the bad one stored in the tabs collection. Let me know if it does!
Thanks Mark. Isabel can you test this patch? Let me know if you need any help.
Flags: needinfo?(irios.mozilla)
Thanks Mark. Sure Dave! I will test this patch and let you both know.
Flags: needinfo?(irios.mozilla)
Hi Mark,

This is working fine!\o/ thanks for the patch.

Dave, so far I only got it working if I set the device's name via UI. Using a launchArgument, I see the device's name changed but the change is not seen on desktop so the test fails. I keep looking into that while this patch lands.

Thank you for your help!
Priority: -- → P1
Attachment #8985710 - Flags: review?(tchiovoloni)
Assignee: nobody → markh
Status: NEW → ASSIGNED
Comment on attachment 8985710 [details] [diff] [review]
tabs.patch

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

Looks good to me!
Attachment #8985710 - Flags: review?(tchiovoloni) → review+
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a5a04f2906
TPS - Tabs: allow profile property be optional or at least match variable profile. r=tchiovoloni
Keywords: checkin-needed
Backed out changeset 69a5a04f2906 (bug 1466899) for ESlint failure at services/sync/tps/extensions/tps/resource/modules/tabs.jsm

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6f1b6284f474a08953cd3b58709c14c5cd5e684

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=86c39d3d96f91334345398c2f3d74193a09e3d66&selectedJob=183908219

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=183908219&repo=mozilla-inbound&lineNumber=258

[task 2018-06-20T02:25:58.068Z] Installing setuptools, pip, wheel...done.
[task 2018-06-20T02:25:59.169Z] running build_ext
[task 2018-06-20T02:25:59.169Z] building 'psutil._psutil_linux' extension
[task 2018-06-20T02:25:59.169Z] creating build
[task 2018-06-20T02:25:59.169Z] creating build/temp.linux-x86_64-2.7
[task 2018-06-20T02:25:59.169Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-06-20T02:25:59.169Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-06-20T02:25:59.169Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-06-20T02:25:59.169Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-06-20T02:25:59.169Z] creating build/lib.linux-x86_64-2.7
[task 2018-06-20T02:25:59.169Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-06-20T02:25:59.169Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-06-20T02:25:59.169Z] building 'psutil._psutil_posix' extension
[task 2018-06-20T02:25:59.169Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-06-20T02:25:59.169Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-06-20T02:25:59.169Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-06-20T02:25:59.169Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-06-20T02:25:59.169Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-06-20T02:25:59.169Z] 
[task 2018-06-20T02:25:59.169Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-06-20T02:31:45.863Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/services/sync/tps/extensions/tps/resource/modules/tabs.jsm:75:57 | Missing semicolon. (semi)
[taskcluster 2018-06-20 02:31:46.253Z] === Task Finished ===
[taskcluster 2018-06-20 02:31:46.253Z] Unsuccessful task run with exit code: 1 completed in 607.663 seconds
Flags: needinfo?(markh)
Attachment #8985710 - Attachment is obsolete: true
Flags: needinfo?(markh)
Sorry about that!
Keywords: checkin-needed
Tried to land the patch but I got the following conflict:

mozilla@ubuntu:~/mozilla-unified$ hg qimport bz://1466899
Fetching... done
Parsing... done
adding 1466899 to series file
renamed 1466899 -> t.patch
mozilla@ubuntu:~/mozilla-unified$ hg qseries
t.patch
mozilla@ubuntu:~/mozilla-unified$ hg qpush -a
applying t.patch
unable to find 'sync/tps/extensions/tps/resource/modules/tabs.jsm' for patching
(use '--prefix' to apply patch relative to the current directory)
1 out of 1 hunks FAILED -- saving rejects to file sync/tps/extensions/tps/resource/modules/tabs.jsm.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh t.patch
Flags: needinfo?(markh)
Comment on attachment 8987703 [details]
Bug 1466899 - TPS now checks the name from the clients collection rather than the tabs collection. r=tchiovoloni

Thom Chiovoloni [:tcsc] has approved the revision.

https://phabricator.services.mozilla.com/D1819
Attachment #8987703 - Flags: review+
Pushed by mhammond@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40eebd227af7
TPS now checks the name from the clients collection rather than the tabs collection. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/40eebd227af7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Flags: needinfo?(markh)
You need to log in before you can comment on or make changes to this bug.