Add a tag to manifest files to denote the test's use of external/internet resources

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: zcampbell, Assigned: zcampbell)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

46 bytes, text/x-github-pull-request
davehunt
: review+
rwood
: review+
Details | Review
46 bytes, text/x-github-pull-request
Bebe
: review+
AndreiH
: feedback+
Details | Review
(Assignee)

Description

5 years ago
Let's see if we can obsolete the tbpl-manifest.ini file and use the main manifest file instead.
(Assignee)

Updated

5 years ago
Assignee: nobody → zcampbell
(Assignee)

Comment 1

5 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

5 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

5 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)
(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

5 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

5 years ago
Summary: Refactor to obsolete the tbpl-manfest file → Refactor to obsolete the tbpl-manifest file
(Assignee)

Comment 6

5 years ago
Posted file github pr
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] [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

5 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?
(Assignee)

Comment 10

5 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 on attachment 8475114 [details] [review]
github pr

Looks great
Attachment #8475114 - Flags: review?(rwood) → review+
Attachment #8475114 - Flags: review?(dave.hunt) → review+
(Assignee)

Updated

5 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)

Updated

5 years ago
Blocks: 1056007
(Assignee)

Comment 12

5 years ago
Merged, thankyou!

https://github.com/mozilla-b2g/gaia/commit/2433b5627567eaa81bee1cb9273b885e2a4a2ed8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

4 years ago
Updating to v2.0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

4 years ago
Posted file github pr for v2.0
Attachment #8534926 - Flags: review?(florin.strugariu)
Attachment #8534926 - Flags: feedback?(andrei.hutusoru)

Comment 15

4 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+
Attachment #8534926 - Flags: review?(florin.strugariu) → review+
Uplift merged in https://github.com/mozilla-b2g/gaia/commit/3deab7205f92cfad10f03a444fcdc7fbeee8e6b4
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.