Closed
Bug 1044484
Opened 10 years ago
Closed 10 years ago
Add a tag to manifest files to denote the test's use of external/internet resources
Categories
(Firefox OS Graveyard :: Gaia::UI Tests, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zcampbell, Assigned: zcampbell)
References
Details
Attachments
(2 files)
Let's see if we can obsolete the tbpl-manifest.ini file and use the main manifest file instead.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → zcampbell
Assignee | ||
Comment 1•10 years ago
|
||
I've experimented with this a bit on TBPL.
As it is, running functional/manifest.ini on TBPL will pass and work, but this is in breach of TBPL guidelines requiring that tests do not use internet resources.
We cannot simply do --type=b2g-online or --type=b2g-wifi-carrier because some tests for example (test_browser_navigation.py) are tagged in a way that they require both internet and local resources, depending upon whether it is running on device or on desktopb2g.
The manifestparser does not allow you to parse tags (ie online = device == "desktop") so we cannot use logic resolve this anomaly.
The online tag has already been pre-defined to mean "connected" rather than "online to the internet" so it is not accurate to say that a test tagged with online uses internet resources.
It seems like we need yet another tag to mean "internet", but I am loathe to add another one. There are already many and they are difficult and not used with success by Gip users.
Alternatively, modify some tests to run only from local resources and avoid the manifest tag clash of using local resources and internet resources in the same test.
Assignee | ||
Comment 2•10 years ago
|
||
If we just ignore Travis and local running, which I failed to mention in comment #2, has desktopb2g and allows internet resources then we can just add:
skip-if = device == "desktop" for the tests that use external resources, effectively limiting them to device only.
For local running one can always call the .py file directly and skip the use of the manifest parser.
Assignee | ||
Comment 3•10 years ago
|
||
Also adding a new `internet` (or however named) tag will require us to modify the TBPL jobs and synchronise the landing of patches so not to break builds.
Dave considering the issues here, what do you feel is the best option?
Flags: needinfo?(dave.hunt)
Comment 4•10 years ago
|
||
(In reply to Zac C (:zac) from comment #3)
> Also adding a new `internet` (or however named) tag will require us to
> modify the TBPL jobs and synchronise the landing of patches so not to break
> builds.
>
> Dave considering the issues here, what do you feel is the best option?
I think adding a tag to indicate that there is a need for access outside of the local machine is likely the best way to go. This could be remote=true or external=true. The command line would change to --type=b2g-remote (for example). We should be able to do this without any disruptions by landing the new manifest tag changes, followed by the mozharness command line changes to use the main manifest, followed by the removal of the tbpl manifest.
Flags: needinfo?(dave.hunt)
Assignee | ||
Comment 5•10 years ago
|
||
Ok I'm coming around to this way too. I'll start on the pull request this afternoon.
It'd be even better to combine this with 1054307 to limit the amount of tags and clashing tags we'll need.
Assignee | ||
Updated•10 years ago
|
Summary: Refactor to obsolete the tbpl-manfest file → Refactor to obsolete the tbpl-manifest file
Assignee | ||
Comment 6•10 years ago
|
||
I decided to use `internet` as the name instead of `remote` as remote just seemed a bit vague. Remote to what? remoteresources was a bit too long. internet just seemed to be straight to the point.
Also unfortunately because of the way some browser tests use both local and internet resources the test has to be tagged with both. It means we'll probably lose this test coverage on TBPL unless we sort out bug 1054307 in the meantime.
Attachment #8475114 -
Flags: review?(rwood)
Attachment #8475114 -
Flags: review?(dave.hunt)
Comment 7•10 years ago
|
||
Comment on attachment 8475114 [details] [review]
github pr
I don't like the idea of using 'internet' as it seems unnecessarily specific. Really all we want to indicate is that it's not entirely bound to the local machine. We should avoid using local=false though as that would require us to set a default of local=true. Using remote or external would be consistent with other test harnesses.
The tags indicate a requirement from the test, so if a test can run with or without a remote connection (such as the browser tests) then we shouldn't add the tag.
Also, we should update the documentation with this change.
Attachment #8475114 -
Flags: review?(dave.hunt) → review-
Assignee | ||
Comment 8•10 years ago
|
||
IMO using remote or external is the opposite, too vague. you'd need to append "resource" or "connection" to it to be more clear. --type=b2g-remoteresource
Within reasonable bounds of this test suite, what could possibly be remote that's not on the internet?
Comment 9•10 years ago
|
||
Comment on attachment 8475114 [details] [review]
github pr
Missed this one:
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/contacts/manifest.ini#L42
Attachment #8475114 -
Flags: review?(rwood) → review-
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8475114 [details] [review]
github pr
I've submitted a new commit that addresses comments and changes internet to external.
Attachment #8475114 -
Flags: review?(rwood)
Attachment #8475114 -
Flags: review?(dave.hunt)
Attachment #8475114 -
Flags: review-
Comment 11•10 years ago
|
||
Comment on attachment 8475114 [details] [review]
github pr
Looks great
Attachment #8475114 -
Flags: review?(rwood) → review+
Updated•10 years ago
|
Attachment #8475114 -
Flags: review?(dave.hunt) → review+
Assignee | ||
Updated•10 years ago
|
Summary: Refactor to obsolete the tbpl-manifest file → Add a tag to manifest files to denote the test's use of external/internet resources
Assignee | ||
Comment 12•10 years ago
|
||
Merged, thankyou!
https://github.com/mozilla-b2g/gaia/commit/2433b5627567eaa81bee1cb9273b885e2a4a2ed8
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•10 years ago
|
||
Updating to v2.0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8534926 -
Flags: review?(florin.strugariu)
Attachment #8534926 -
Flags: feedback?(andrei.hutusoru)
Comment 15•10 years ago
|
||
Comment on attachment 8534926 [details] [review]
github pr for v2.0
The uplift looks good. Thanks zac!
Attachment #8534926 -
Flags: feedback?(andrei.hutusoru) → feedback+
Updated•10 years ago
|
Attachment #8534926 -
Flags: review?(florin.strugariu) → review+
Comment 16•10 years ago
|
||
Uplift merged in https://github.com/mozilla-b2g/gaia/commit/3deab7205f92cfad10f03a444fcdc7fbeee8e6b4
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•