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)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zcampbell, Assigned: zcampbell)

References

Details

Attachments

(2 files)

(deleted), text/x-github-pull-request
davehunt
: review+
rwood
: review+
Details
(deleted), text/x-github-pull-request
Bebe
: review+
AndreiH
: feedback+
Details
Let's see if we can obsolete the tbpl-manifest.ini file and use the main manifest file instead.
Assignee: nobody → zcampbell
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.
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.
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)
(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)
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.
Summary: Refactor to obsolete the tbpl-manfest file → Refactor to obsolete the tbpl-manifest file
Attached file github pr (deleted) —
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 on attachment 8475114 [details] 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-
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 on attachment 8475114 [details] 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 on attachment 8475114 [details] github pr Looks great
Attachment #8475114 - Flags: review?(rwood) → review+
Attachment #8475114 - Flags: review?(dave.hunt) → review+
Summary: Refactor to obsolete the tbpl-manifest file → Add a tag to manifest files to denote the test's use of external/internet resources
Blocks: 1056007
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updating to v2.0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file github pr for v2.0 (deleted) —
Attachment #8534926 - Flags: review?(florin.strugariu)
Attachment #8534926 - Flags: feedback?(andrei.hutusoru)
Comment on attachment 8534926 [details] github pr for v2.0 The uplift looks good. Thanks zac!
Attachment #8534926 - Flags: feedback?(andrei.hutusoru) → feedback+
Attachment #8534926 - Flags: review?(florin.strugariu) → review+
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: