Remove android*.json

RESOLVED FIXED in Firefox 39

Status

Testing
Mochitest
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: gbrown, Assigned: vaibhav1994)

Tracking

(Depends on: 1 bug)

Trunk
mozilla39
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [ateam_harness_work])

Attachments

(7 attachments, 19 obsolete attachments)

31.57 KB, patch
gbrown
: review+
Details | Diff | Splinter Review
7.83 KB, patch
vaibhav1994
: review+
Details | Diff | Splinter Review
6.87 KB, patch
vaibhav1994
: review+
Details | Diff | Splinter Review
2.51 KB, patch
armenzg
: review+
Details | Diff | Splinter Review
4.13 KB, patch
armenzg
: review+
Details | Diff | Splinter Review
11.60 KB, patch
jmaher
: review+
gbrown
: review+
Details | Diff | Splinter Review
5.83 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
We use testing/mochitest/android.json, android23.json, and androidx86.json to differentiate between Android 4.0 / Android 2.3 / Android x86 mochitests. Instead, we should have appropriate annotations in the normal mochitest.ini manifests.

I think Android x86 can be identified as os: 'android' && processor: 'x86'.

Differentiating 2.3 and 4.0 is more difficult. See http://hg.mozilla.org/mozilla-central/annotate/62f0b771583c/testing/mochitest/runtestsremote.py#l642 for some prior work to provide android_version for manifests. This is added at test run time -- it actually checks the run-time environment to determine what version of android is in use on the test device. As I recall, this is problematic because manifest annotations are also evaluated at build time.
(Assignee)

Comment 1

3 years ago
Ok, so lets tackle this in pieces. Since the manifest annotations are evaluated at build time, should I first start with making it run from test harness? I would appreciate if I get some pointers to start on this one.
(Reporter)

Comment 2

3 years ago
If you want to start with something straight-forward, I would try to eliminate androidx86.json first: We should be able to use 'os' and 'processor' annotations in mochitest.ini.

I'm not 100% sure that there is an issue with using 'android_version'. Maybe it would be useful if you pushed a quick test to try, with just a few android_version instances -- does it break anything? does it work as intended?
(Assignee)

Comment 3

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=102324d67d28 - A push to know the mozinfo.info config for android so that we can go forward with determining appropriate annotations in manifest.
On a positive note, as of bug 1074507 we no longer do any build-time test filtering! That means you should be able to use the android_version key with impunity, as long as it's implemented properly in each harness.
(Assignee)

Comment 5

3 years ago
So I checked for x86 mozinfo: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vaibhavmagarwal@gmail.com-102324d67d28/try-android-x86/fennec-36.0a1.en-US.android-i386.mozinfo.json

'os' and 'toolkit' == 'android' and the 'processor' == 'x86'.

So for x86.json annotation, I would personally prefer

skip-if = toolkit == 'android' && processor == 'x86' because in all annotations previously ( Bug 970925 ), we have used 'toolkit', so for the sake of being consistency.
(Assignee)

Comment 6

3 years ago
Created attachment 8509448 [details] [diff] [review]
androidx86.patch

Patch for androidx86.json
This contains entries that were straight-forward and there are still ~25 entries remaining in androidx86.json, which have to require careful changing to manifests.
thanks for this patch!  Any chance you have a try push?
(Reporter)

Comment 8

3 years ago
(In reply to Joel Maher (:jmaher) from comment #7)
> Any chance you have a try push?

We only run Android x86 xpcshell and robocop tests on trunk trees, so a push to Cedar might be more useful -- I can do that.
(Reporter)

Comment 9

3 years ago
There is a problem -- this breaks builds on all platforms. See https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=89587e237cb4.

Traceback (most recent call last):
  File "./config.status", line 968, in <module>
    config_status(**args)
  File "/builds/slave/ced-and-x86-000000000000000000/build/python/mozbuild/mozbuild/config_status.py", line 148, in config_status
    summary = the_backend.consume(definitions)
  File "/builds/slave/ced-and-x86-000000000000000000/build/python/mozbuild/mozbuild/backend/base.py", line 181, in consume
    for obj in objs:
  File "/builds/slave/ced-and-x86-000000000000000000/build/python/mozbuild/mozbuild/frontend/emitter.py", line 137, in emit
    objs = list(self.emit_from_context(out))
  File "/builds/slave/ced-and-x86-000000000000000000/build/python/mozbuild/mozbuild/frontend/emitter.py", line 783, in emit_from_context
    for obj in self._process_test_manifest(context, info, path):
  File "/builds/slave/ced-and-x86-000000000000000000/build/python/mozbuild/mozbuild/frontend/emitter.py", line 977, in _process_test_manifest
    context)
mozbuild.frontend.reader.SandboxValidationError: 
==============================
ERROR PROCESSING MOZBUILD FILE
==============================

The error occurred while processing the following file or one of the files it includes:

    /builds/slave/ced-and-x86-000000000000000000/build/content/media/moz.build

The error occurred when validating the result of the execution. The reported error is:

    Error processing test manifest file /builds/slave/ced-and-x86-000000000000000000/build/content/media/test/mochitest.ini: Traceback (most recent call last):
    
      File "/builds/slave/ced-and-x86-000000000000000000/build/python/mozbuild/mozbuild/frontend/emitter.py", line 840, in _process_test_manifest
        m = manifestparser.TestManifest(manifests=[path], strict=True)
    
      File "/builds/slave/ced-and-x86-000000000000000000/build/testing/mozbase/manifestparser/manifestparser/manifestparser.py", line 416, in __init__
        self.read(*manifests)
    
      File "/builds/slave/ced-and-x86-000000000000000000/build/testing/mozbase/manifestparser/manifestparser/manifestparser.py", line 572, in read
        self._read(here, filename, defaults)
    
      File "/builds/slave/ced-and-x86-000000000000000000/build/testing/mozbase/manifestparser/manifestparser/manifestparser.py", line 467, in _read
        sections = read_ini(fp=fp, variables=defaults, strict=self.strict)
    
      File "/builds/slave/ced-and-x86-000000000000000000/build/testing/mozbase/manifestparser/manifestparser/manifestparser.py", line 364, in read_ini
        assert key not in current_section
    
    AssertionError
(Assignee)

Comment 10

3 years ago
Created attachment 8509714 [details] [diff] [review]
androidx86.patch

:gbrown, yes I was testing it out locally. Sorry I did not see your comment and hence did not reply to it (was busy debugging locally). I have attached the new patch that is now building locally for me.
Attachment #8509448 - Attachment is obsolete: true
(Reporter)

Comment 11

3 years ago
Comment on attachment 8509448 [details] [diff] [review]
androidx86.patch

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

::: content/media/test/mochitest.ini
@@ +318,1 @@
>  skip-if = buildapp == 'mulet' || os == 'win' # bug 894922

Is this valid -- having more than one skip-if for a test?
Attachment #8509448 - Attachment is obsolete: false
(Assignee)

Comment 12

3 years ago
Cedar push by :gbrown https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=8d185e7c78ee
(Assignee)

Comment 13

3 years ago
Created attachment 8510242 [details] [diff] [review]
androidx86_continued.patch

Continuing annotations for the remaining entries in androidx86.json.
Just one entry for 'robocop' remaining after this in excludetests, as I am not sure of it. In this patch, I have removed the entries of which there are no files present and have not annotated the manifest separately for those tests which have:
skip-if = toolkit == 'android'
as they are skipped for all of android.
Attachment #8509448 - Attachment is obsolete: true
Comment on attachment 8510242 [details] [diff] [review]
androidx86_continued.patch

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

just a drive by review, lots of great progress.

::: dom/canvas/test/webgl-mochitest.ini
@@ +1,2 @@
>  [DEFAULT]
> +skip-if = (toolkit == 'android' && processor == 'x86') #bug 865443 - separate suite -- mochitest-gl

isn't this the case for all of android?

::: dom/devicestorage/ipc/mochitest.ini
@@ +1,3 @@
>  [DEFAULT]
>  skip-if = toolkit == 'android' || e10s #bug 781789 & bug 782275
> +skip-if = (toolkit == 'android' && processor == 'x86') #bug 781789 & bug 782275

we can't have >1 skip-if line per section

::: dom/devicestorage/test/mochitest.ini
@@ +1,3 @@
>  [DEFAULT]
>  skip-if = e10s # bug 781789 & bug 782275
> +skip-if = (toolkit == 'android' && processor == 'x86') #bug 781789 & bug 782275

need a single skip-if
(Assignee)

Comment 15

3 years ago
Created attachment 8510283 [details] [diff] [review]
androidx86_continued.patch

:jmaher thanks for the review! I have corrected the nits.

:gbrown, could you push this patch to cedar? Thanks!
Attachment #8510242 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
Comment on attachment 8509714 [details] [diff] [review]
androidx86.patch

The results for the push: https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=8d185e7c78ee is ok I think, although there are failing chunks, but they are also in a previous push: https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=b66067d8c820

:gbrown, are the results ok according to you? Also, it would be great if you could review the patch. Adding you as the reviewer.
Attachment #8509714 - Flags: review?(gbrown)
(Reporter)

Comment 17

3 years ago
Comment on attachment 8509714 [details] [diff] [review]
androidx86.patch

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

The Cedar results look okay to me. There are some pre-existing crashes, so we lose some information, but I don't think we should let that block us here.

::: content/media/test/mochitest.ini
@@ +315,3 @@
>  [test_buffered.html]
>  [test_bug448534.html]
> +skip-if = buildapp == 'mulet' || os == 'win' || (toolkit == 'android' && processor == 'x86') # bug 894922 #x86 only bug 914439

Is "# ... # ..." legal? I guess so, but I think I would prefer a , or ; or something like that. (There are a few of these.)
Attachment #8509714 - Flags: review?(gbrown) → review+
(Reporter)

Comment 18

3 years ago
Comment on attachment 8510283 [details] [diff] [review]
androidx86_continued.patch

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

I had a quick look here, just to give you some feedback...

::: content/base/test/chrome.ini
@@ +1,5 @@
>  [DEFAULT]
>  
>  [test_bug357450.js]
>  [test_copypaste.xul]
> +skip-if = (toolkit == 'android' && processor == 'x86') #bug 904183

Tests in chrome.ini are run in "mochitest-chrome" test jobs, which do not run on Android. I think test_copypaste.xul was either accidentally added to androidx86.json, or that test previously ran as a regular mochitest but is now a chrome mochitest. Regardless, I would not update chrome.ini files: If a test in android*.json is only in chrome.ini manifests, simply remove it from android*.json.

::: dom/canvas/test/webgl-mochitest.ini
@@ +1,2 @@
>  [DEFAULT]
> +skip-if = toolkit == 'android' #bug 865443 - separate suite -- mochitest-gl

This seems wrong. I think this will skip webgl-mochitest/test-backbuffer-channels.html, etc, for all Android, and that's not part of dom/canvas/test/webgl-conformance.
(Assignee)

Comment 19

3 years ago
Created attachment 8510439 [details] [diff] [review]
androidx86_continued.patch

:gbrown, thanks for pointing out the mistakes! Could you push this patch to cedar for verification? (Just base this patch *above* the previous patch to prevent bitrot)
Attachment #8510283 - Attachment is obsolete: true
(Reporter)

Comment 20

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=3fab6aadcd16
(Assignee)

Comment 21

3 years ago
Created attachment 8510470 [details] [diff] [review]
android23.patch

Patch to clean up android23.patch
(Assignee)

Comment 22

3 years ago
The "robocop" entry is required to be removed in all the android*.json 

:jmaher has suggested, that we should use a [subsuite] keyword by adding a [default] section here: http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/robocop.ini#1 , so that we do not run the tests normally but only when we specify --subsuite=robocop. Is this is the right way to go forward, and if yes, will it require changes in the mozharness scripts?
(Reporter)

Comment 23

3 years ago
I don't understand why we need to treat robocop.ini specially. We have chrome.ini and browser.ini files that are not confused with regular mochitest manifests -- why is robocop.ini different? What happens if you just remove the robocop entry from the android*.json files?
right now we skip robocop in the android*.json file.  For android builds we include robocop/ directory (which includes robocop.ini) in the /tests/ subfolder (tests.zip).  Assuming we remove robocop from the list to exclude, we would run all the robocop tests as part of the plain tests instead of inside their own job.

The idea to use a subsuite would allow us to ignore all robocop jobs unless we specify --subsuite=robocop on the cli to the python test harness.  If I am overlooking something, do speak up, making this easier would be nice!
(Reporter)

Comment 25

3 years ago
The mochitest manifest handling code is too complicated for me to say with any certainty. I'll try to understand this better with an experiment: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=76a9e587375f
(Assignee)

Comment 26

3 years ago
Comment on attachment 8510439 [details] [diff] [review]
androidx86_continued.patch

:gbrown, the try results for this patch look good to me. Adding you as the reviewer for this patch.

Also, from the try push done for robocop, it looks like there is no problem caused by removing robocop entry from android*.json without annotating the manifests, though there maybe some hidden problem.
Attachment #8510439 - Flags: review?(gbrown)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8510470 [details] [diff] [review]
android23.patch

:gbrown, kindly review this patch too and push this to cedar. Apologies for filling up your review queue :)
Attachment #8510470 - Flags: review?(gbrown)
(Assignee)

Updated

3 years ago
Assignee: nobody → vaibhavmagarwal
(Assignee)

Comment 28

3 years ago
Please checkin androidx86.json, it is a r+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/92a162f2d23d
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team][leave open]
(Reporter)

Comment 30

3 years ago
Comment on attachment 8510439 [details] [diff] [review]
androidx86_continued.patch

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

I thought the Cedar run was okay, but I just noticed that mochitest-gl is not running any tests!

http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/cedar-android/1414085296/cedar_ubuntu64_vm_mobile_test-mochitest-gl-bm52-tests1-linux64-build3.txt.gz

16:23:28     INFO -  1 INFO TEST-UNEXPECTED-ERROR | SimpleTest/TestRunner.js | No checks actually run
16:23:28     INFO -  TEST-INFO 
16:23:28     INFO -  2 INFO TEST-START | Shutdown
16:23:28     INFO -  3 INFO Passed:  0
16:23:28     INFO -  4 INFO Failed:  0
16:23:28     INFO -  5 INFO Todo:    0

Everything else looks fine here.
Attachment #8510439 - Flags: review?(gbrown) → review-
(Reporter)

Comment 31

3 years ago
(In reply to Vaibhav (:vaibhav1994) from comment #26)
> Also, from the try push done for robocop, it looks like there is no problem
> caused by removing robocop entry from android*.json without annotating the
> manifests, though there maybe some hidden problem.

I think it is okay to simply remove the robocop entries from android*.json -- everything looks okay to me.
(Reporter)

Comment 32

3 years ago
(In reply to Vaibhav (:vaibhav1994) from comment #27)
> Comment on attachment 8510470 [details] [diff] [review]
> android23.patch
> 
> :gbrown, kindly review this patch too and push this to cedar. Apologies for
> filling up your review queue :)

https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=3c1115f0a0a0

This is not a problem -- keep those patches coming!!
(Assignee)

Comment 33

3 years ago
Created attachment 8511168 [details] [diff] [review]
androidx86_continued.patch

Nice observation :gbrown! I think this was probably because I mistakenly did:

skip = toolkit == 'android' for webgl-conformance tests thus it is skipping for all jobs

I have now replaced it with:

skip-if = (toolkit == 'android' && processor == 'x86') || android_version == "10" || android_version == "15"

to skip for android.json, android23.json and androidx86.json

Kindly push this patch to cedar if you think the logic is fine (also removed the robocop entries in this patch from android8.json since the results are as expected for them)
Attachment #8510439 - Attachment is obsolete: true
(Reporter)

Comment 34

3 years ago
(In reply to Vaibhav (:vaibhav1994) from comment #33)
> Created attachment 8511168 [details] [diff] [review]
> androidx86_continued.patch
> 
> Nice observation :gbrown! I think this was probably because I mistakenly did:
> 
> skip = toolkit == 'android' for webgl-conformance tests thus it is skipping
> for all jobs
> 
> I have now replaced it with:
> 
> skip-if = (toolkit == 'android' && processor == 'x86') || android_version ==
> "10" || android_version == "15"
> 
> to skip for android.json, android23.json and androidx86.json

I don't see how that will help Android 2.3/4.0 mochitest-gl. I'm not sure what is going wrong actually, so maybe this is worth a try...but I'm skeptical. :)

> Kindly push this patch to cedar if you think the logic is fine (also removed
> the robocop entries in this patch from android8.json since the results are
> as expected for them)

Cedar should only be necessary for checking on the Android x86 mochitests. Cedar is less efficient than try in that it starts builds for all platforms by default. Would you mind pushing to try instead? When we think everything is sorted out, I'll push a final version to Cedar to double-check on x86.
(Assignee)

Comment 35

3 years ago
Created attachment 8511216 [details] [diff] [review]
androidx86_continued.patch

I have left web-conformance entry in android*.json and removed all other entries in this patch, so that we can push this in the meanwhile we find a solution for web-conformance entry.

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=48615c239c33
Attachment #8511168 - Attachment is obsolete: true
(Reporter)

Comment 36

3 years ago
(In reply to Geoff Brown [:gbrown] from comment #32)
> https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=3c1115f0a0a0

Oops -- all the Android 2.3 mochitests failed here!
https://hg.mozilla.org/mozilla-central/rev/92a162f2d23d
Whiteboard: [fixed-in-fx-team][leave open] → [leave open]
(Reporter)

Comment 38

3 years ago
Comment on attachment 8510470 [details] [diff] [review]
android23.patch

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

This *looks* okay to me, but failed on Cedar. r- pending a successful try run.
Attachment #8510470 - Flags: review?(gbrown) → review-
(Assignee)

Comment 39

3 years ago
Error seen in try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=48615c239c33

12:41:41     INFO -  10-24 12:35:41.882 I/GeckoDump( 2256): TEST-UNEXPECTED-FAIL: setup.js | error parsing http://mochi.test:8888/android.json (SyntaxError: JSON.parse: expected double-quoted property name at line 5 column 3 of the JSON data)
12:41:41     INFO -  10-24 12:35:41.882 W/GeckoConsole( 2256): [JavaScript Error: "SyntaxError: JSON.parse: expected double-quoted property name at line 5 column 3 of the JSON data" {file: "http://mochi.test:8888/manifestLibrary.js" line: 49}]


Forgot to remove a comma at the end in the *.json in androidx86_continued.patch.
A new try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=901eeb3462c2
(Assignee)

Comment 40

3 years ago
Another push to try for removing android23.json: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=600649c4dec1
(Assignee)

Comment 41

3 years ago
Created attachment 8513718 [details] [diff] [review]
android23.patch

The android jobs are green in the try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=600649c4dec1 (ignore the bc jobs on linux, they are because of a patch that has now been backed out from mozilla inbound).

:gbrown, kindly push to cedar if you feel necessary. Adding you as the reviewer for this patch.
Attachment #8510470 - Attachment is obsolete: true
Attachment #8513718 - Flags: review?(gbrown)
(Reporter)

Comment 42

3 years ago
Comment on attachment 8513718 [details] [diff] [review]
android23.patch

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

The try run looks good to me, except I am not sure about Android 2.3 mochitest-gl -- I re-triggered.

r+ with nit addressed and assuming those retries are okay.

::: docshell/test/navigation/mochitest.ini
@@ +43,5 @@
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage
>  [test_popup-navigates-children.html]
>  skip-if = buildapp == 'b2g' # b2g(Needs multiple window.open support, also uses docshelltreenode) b2g-debug(Needs multiple window.open support, also uses docshelltreenode) b2g-desktop(Needs multiple window.open support, also uses docshelltreenode)
>  [test_reserved.html]
> +skip-if = (buildapp == 'b2g' && (toolkit != 'gonk' || debug)) || android_version == "10" #too slow on Android 2.3 aws only; bug 1030403

nit -- It looks to me like nearly all existing skip-if operations use single quotes, rather than double quotes: ' vs ". I think you should use single quotes, to be consistent.
Attachment #8513718 - Flags: review?(gbrown) → review+
(Reporter)

Comment 43

3 years ago
(In reply to Vaibhav (:vaibhav1994) from comment #41)
> :gbrown, kindly push to cedar if you feel necessary.

I don't think it is necessary since there is nothing x86-specific here.
(Assignee)

Comment 44

3 years ago
Created attachment 8514009 [details] [diff] [review]
android23.patch

Carrying forward the r+

Mochitest-gl job is passing in the try push, log: http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/vaibhavmagarwal@gmail.com-600649c4dec1/try-android/try_ubuntu64_vm_mobile_test-mochitest-gl-bm118-tests1-linux64-build509.txt.gz
Attachment #8513718 - Attachment is obsolete: true
Attachment #8514009 - Flags: review+
(Assignee)

Comment 45

3 years ago
Created attachment 8514011 [details] [diff] [review]
androidx86_continued.patch

This patch is passing on try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=21663d609d81

:gbrown, please push to cedar to test for androidx86.
Attachment #8511216 - Attachment is obsolete: true
(In reply to Geoff Brown [:gbrown] from comment #42)
> nit -- It looks to me like nearly all existing skip-if operations use single
> quotes, rather than double quotes: ' vs ". I think you should use single
> quotes, to be consistent.

The manifest expression parser supports both single and double-quoted strings, I don't really know why (probably because both Python and JS do). We could narrow that down to just single-quoted strings to make things forcibly consistent.
(Reporter)

Comment 47

3 years ago
(In reply to Vaibhav (:vaibhav1994) from comment #45)
> :gbrown, please push to cedar to test for androidx86.

https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=61a1a25cdd84
(Assignee)

Comment 48

3 years ago
Created attachment 8514781 [details] [diff] [review]
androidx86_continued.patch

The try push: https://treeherder.mozilla.org/ui/#/jobs?repo=cedar&revision=61a1a25cdd84 results look good.

:gbrown, please review this patch.
Attachment #8514011 - Attachment is obsolete: true
Attachment #8514781 - Flags: review?(gbrown)
(Reporter)

Comment 49

3 years ago
Comment on attachment 8514781 [details] [diff] [review]
androidx86_continued.patch

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

::: testing/mochitest/androidx86.json
@@ -15,5 @@
> -  "dom/imptests/html/webgl": "WebGL",
> -  "dom/imptests/webapps/WebStorage/tests/submissions/Infraware/test_storage_local_key.html": "bug 775227",
> -  "dom/inputmethod": "Not supported on Android",
> -  "dom/media/test/test_play_twice.html": "x86 only bug 914439",
> -  "dom/media/test/test_played.html": "bug 751539",

It looks like some tests removed from here (test_played, test_play_twice) are not added to any manifest. Why is that?
(Assignee)

Comment 50

3 years ago
(In reply to Geoff Brown [:gbrown] from comment #49)

> ::: testing/mochitest/androidx86.json
> @@ -15,5 @@
> > -  "dom/imptests/html/webgl": "WebGL",
> > -  "dom/imptests/webapps/WebStorage/tests/submissions/Infraware/test_storage_local_key.html": "bug 775227",
> > -  "dom/inputmethod": "Not supported on Android",
> > -  "dom/media/test/test_play_twice.html": "x86 only bug 914439",
> > -  "dom/media/test/test_played.html": "bug 751539",
> 
> It looks like some tests removed from here (test_played, test_play_twice)
> are not added to any manifest. Why is that?

All these tests have "skip-if = toolkit == android" in the manifest. So I did not add it as it would become redundant.
(Reporter)

Comment 51

3 years ago
Right - I see that now.
(Reporter)

Updated

3 years ago
Attachment #8514781 - Flags: review?(gbrown) → review+
(Assignee)

Comment 52

3 years ago
Created attachment 8515107 [details] [diff] [review]
androidx86_continued.patch

Carrying forward the r+. Fixed for bitrot.
Attachment #8514781 - Attachment is obsolete: true
Attachment #8515107 - Flags: review+
(Assignee)

Comment 53

3 years ago
Created attachment 8515108 [details] [diff] [review]
android23.patch

Carrying forward the r+. Fixed for bitrot.
Attachment #8514009 - Attachment is obsolete: true
Attachment #8515108 - Flags: review+
(Assignee)

Comment 54

3 years ago
Kindly checkin android23.patch and androidx86_continued.patch. Thanks
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/667a5bff1599
Keywords: checkin-needed
hi vaibhav, the 23patch didn't apply cleanly (i guess conflicted with the androidx86 patch. 

renamed 1083347 -> android23.patch
applying android23.patch
patching file testing/mochitest/android23.json
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file testing/mochitest/android23.json.rej

could you take a look, thanks!
Flags: needinfo?(vaibhavmagarwal)
https://hg.mozilla.org/mozilla-central/rev/667a5bff1599
(Assignee)

Comment 58

3 years ago
Created attachment 8516090 [details] [diff] [review]
android23.patch

Fixed!
Attachment #8515108 - Attachment is obsolete: true
Flags: needinfo?(vaibhavmagarwal)
Attachment #8516090 - Flags: review+
(Assignee)

Comment 59

3 years ago
For android23.patch
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dad6c124f2a
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4dad6c124f2a
(Assignee)

Comment 62

3 years ago
Now, only 1 entry is left in android*.json: http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/androidx86.json#4
and we cannot simply remove this using something like 'skip-if = toolkit == android', as the android gl jobs run those tests: http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/gl.json#2

Afaik, we cannot differentiate between the android gl and normal android chunks. One of the solution can be to add another keyword to mozinfo for gl jobs like we do for android_version: http://hg.mozilla.org/mozilla-central/annotate/62f0b771583c/testing/mochitest/runtestsremote.py#l642
and then not skip for gl.

Other way might be to use a subsuite tag like we do for browser-chrome tests: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/layoutview/test/browser.ini#3

I am not sure what would be the correct way to fix this, and it would be great if someone could give ideas regarding fixing this!
(Assignee)

Comment 63

3 years ago
:gbrown could you give some advice as to how I could go about fixing this?
Flags: needinfo?(gbrown)
(Reporter)

Comment 64

3 years ago
I have not thought this out fully, but as a guideline, I would like the Android mochitest-gl suite to run the same way as it does on Linux, Mac, etc., if that is possible.

From test logs for mochitest-gl on other platforms, it looks like those other platforms use --manifest=mochitest-subsuite-webgl.ini instead of --test-manifest=gl.json. I don't immediately see how those tests are excluded from the main batch of mochitests though. 

Bug 1064432 changed some things here -- that patch looks interesting.
Flags: needinfo?(gbrown)
we could push to try and test this.  One way to do this without editing mozharness would be to:
* hack runtestsremote.py and if --test-manifest=='gl.json', set --manifest='mochitest-subsuite-webgl.ini' and any other differences
* remove gl.json from android*.json

I suspect there is one or two details in there that might need addressing, but within a few try pushes we could have a patch for m-c and mozharness to solve this.  The problem is that we would need to get this on mozilla-aurora and mozilla-beta so the mozharness changes would work.

Lets start with a patch that hacks the changes we need in place, then we can figure out a deployment.
(Reporter)

Comment 66

3 years ago
Recent changes in bug 1064002 might make deployment easier.
https://hg.mozilla.org/projects/ash/rev/85b5f8310a30
I landed the change to mozharness which will allow testing this on Try:
https://tbpl.mozilla.org/?tree=Ash&rev=6a32858f85ff

It needs to be merged to the production branch.
In any case, we can always re-trigger once it merges.

:vaibhav1994 feel free to land on try.
Trigger all Android unit test jobs. I want to review the jobs once you do so.
The mozharness code is now live.
You can push to try and let me know.
(Assignee)

Comment 71

2 years ago
A test push to try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d94c8bca20e2
(In reply to Vaibhav (:vaibhav1994) from comment #71)
> A test push to try:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d94c8bca20e2

I re-triggered your jobs because the code run against the wrong revision of mozharness (my bad!).

Your re-triggered jobs will say:
https://hg.mozilla.org/build/mozharness/rev/4c021248ba40
This time it used the right mozharness code but it failed.

For the record, my change got backed out.
You will probably want to push to try by modifying testing/mozharness/mozharness.json and pointing it to my user repo (http://hg.mozilla.org/users/armenzg_mozilla.com/mozharness) until my code lands again.
(Reporter)

Comment 74

2 years ago
(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #73)
> This time it used the right mozharness code but it failed.

It looks like all the jobs tried to run all of the tests, constrained only by chunk: non-webgl tests ran even though --manifest was passed on the runtestsremote.py command line. Do we know why?
In bug 1064002 I discovered that if --test-manifest is not the last parameter we fail.
I'm trying to figure out what is needed to work around it as it is my last blocking issue.

(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #86)
> It seems that for some harnesses the order of parameters can be troublesome.
> [1]
> 
> This fails:
> --total-chunks=2 --test-manifest=gl.json --this-chunk=1
> 
> This succeeds:
> --total-chunks=2 --this-chunk=1 --test-manifest=gl.json
> 
> [1]
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bd4f7fbc017b
> /builds/slave/test/build/venv/bin/python -u
> /builds/slave/test/build/tests/mochitest/runtestsremote.py --autorun
> --close-when-done --dm_trans=sut --console-level=INFO
> --app=org.mozilla.fennec --remote-webserver=10.0.2.2
> --xre-path=/builds/slave/test/build/hostutils/xre
> --utility-path=/builds/slave/test/build/hostutils/bin --deviceIP=127.0.0.1
> --devicePort=20701 --http-port=8854 --ssl-port=4454
> --certificate-path=/builds/slave/test/build/tests/certs
> --symbols-path=https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-
> builds/armenzg@mozilla.com-bd4f7fbc017b/try-android-api-9/fennec-37.0a1.en-
> US.android-arm.crashreporter-symbols.zip --quiet
> --log-raw=/builds/slave/test/build/blobber_upload_dir/raw_structured_logs.
> log --total-chunks=2 --test-manifest=gl.json --this-chunk=1
(Assignee)

Comment 76

2 years ago
Hi! I got involved in other projects in auto-tools in the past month and this bug went unnoticed. I would like to finish up the remaining part in this bug. Previously, there were a lot of changes in mozharness happening, but now I think that they are probably done. By changing this line http://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/android_arm_config.py#34 and making it point to _webgl-conformance.ini does not yield the expected results, so I guess there might be some other way to do it. Armen, it would be really good if you could give me pointers about how to solve this piece of the puzzle. Thanks
Hi Vaibhav,
Happy new year!

Unfortunately, I got rather stuck in bug 1064002.

I will do new try pushes to see where we are. I've lost track of where we are.

I will NI myself to get back to you in the next 2 days. Please CC yourself to the bug.

Btw, can you push to try? or to my user repo? Once I know where we stand I might ask for your help if you would like to.
Depends on: 1064002
Flags: needinfo?(armenzg)
(Assignee)

Comment 78

2 years ago
Armen, thanks for replying. I am ready to do try pushes to test things out, but I am not really sure what changes I have to make before pushing to try. We can discuss the changes here or I can ping you on IRC, whichever you feel comfortable.
I think it might have worked:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecdb1c21ae76&filter-searchStr=gl1

I will double check.
Flags: needinfo?(armenzg)
(Assignee)

Comment 80

2 years ago
The above try push has:

13:57:45     INFO - Running on test-1 the command /builds/slave/test/build/venv/bin/python -u /builds/slave/test/build/tests/mochitest/runtestsremote.py --autorun --close-when-done --dm_trans=sut --console-level=INFO --app=org.mozilla.fennec --remote-webserver=10.0.2.2 --xre-path=/builds/slave/test/build/hostutils/xre --utility-path=/builds/slave/test/build/hostutils/bin --deviceIP=127.0.0.1 --devicePort=20701 --http-port=8854 --ssl-port=4454 --certificate-path=/builds/slave/test/build/tests/certs --symbols-path=https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/armenzg@mozilla.com-ecdb1c21ae76/try-android-api-9/fennec-37.0a1.en-US.android-arm.crashreporter-symbols.zip --quiet --log-raw=/builds/slave/test/build/blobber_upload_dir/raw_structured_logs.log --total-chunks=2 --test-manifest=gl.json --this-chunk=1


So here, --test-manifest=gl.json is *before* --this-chunk=1 and it passes, so it looks like this problem is fixed. Let me know if this has landed and should I make changes to android_arm_config.py or elsewhere.
I'm waiting for ahal's review.
I've also re-triggered a couple of jobs that did not come back green.
I will be landing it on Monday.
I have landed the Mozharness piece.
It will have to be merged to the production branch before it is live.
After that you should be able to work on this. I think.
Do you know have what you need?

All values in this file are now in use on trunk:
https://hg.mozilla.org/integration/mozilla-inbound/file/03d2076110a3/testing/config/mozharness/android_arm_config.py
Flags: needinfo?(vaibhavmagarwal)
(Assignee)

Comment 84

2 years ago
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b513bbe2f4
Flags: needinfo?(vaibhavmagarwal)
(Reporter)

Comment 85

2 years ago
(In reply to Vaibhav (:vaibhav1994) from comment #84)
> Pushed to try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b513bbe2f4

Oh no, that didn't run any android tests. You need "-p android-api-9,android-api-11" now.
(Assignee)

Comment 86

2 years ago
Oops, didn't know the trychooser syntax changed.

Repushed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09f592ec13cb

and retriggered the gl jobs as it is not clear whether the test failures are intermittents or not
(Reporter)

Comment 87

2 years ago
For both Android 2.3 and Android 4.0, the gl tests' runtestsremote.py command line looks correct, but the jobs are actually running non-gl tests, as though --test-manifest is being ignored. Using 2.3 gl1 as an example:

http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/vaibhavmagarwal@gmail.com-09f592ec13cb/try-android-api-9/try_ubuntu64_vm_mobile_test-mochitest-gl-1-bm51-tests1-linux64-build195.txt.gz

Running on test-1 the command /builds/slave/test/build/venv/bin/python -u /builds/slave/test/build/tests/mochitest/runtestsremote.py --autorun --close-when-done --dm_trans=sut --console-level=INFO --app=org.mozilla.fennec --remote-webserver=10.0.2.2 --xre-path=/builds/slave/test/build/hostutils/xre --utility-path=/builds/slave/test/build/hostutils/bin --deviceIP=127.0.0.1 --devicePort=20701 --http-port=8854 --ssl-port=4454 --certificate-path=/builds/slave/test/build/tests/certs --symbols-path=https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/vaibhavmagarwal@gmail.com-09f592ec13cb/try-android-api-9/fennec-38.0a1.en-US.android-arm.crashreporter-symbols.zip --quiet --log-raw=/builds/slave/test/build/blobber_upload_dir/mochitest-gl-1_raw.log --total-chunks=2 --test-manifest=tests/mochitest/tests/dom/canvas/test/_webgl-conformance.ini --this-chunk=1

...

18:21:19     INFO -  Running tests 1-1650/3299
18:21:19     INFO -  0 INFO SimpleTest START
18:21:19     INFO -  1 INFO TEST-START | caps/tests/mochitest/test_app_principal_equality.html
18:21:19     INFO -  2 INFO TEST-OK | caps/tests/mochitest/test_app_principal_equality.html | took 7569ms
18:21:19     INFO -  3 INFO TEST-START | caps/tests/mochitest/test_bug246699.html
18:21:19     INFO -  4 INFO TEST-OK | caps/tests/mochitest/test_bug246699.html | took 4432ms
that is usually a sign of a manifest path which doesn't exist, then we default to the main one.

how can we confirm that this path exists:
tests/mochitest/tests/dom/canvas/test/_webgl-conformance.ini

I downloaded the tests.zip and it appears to be there and in the right path.  Maybe there is some other condition somewhere that is not allowing a custom .ini for remote mochitests?
(Assignee)

Comment 89

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=092179ebafa7

^ Tried with --test-manifest=tests/dom/canvas/test/_webgl-conformance.ini relative to the path where gl.json is present, but it is still failing. Maybe there is some other condition to change..
oh, I think I see the problem:
for desktop linux we use: --manifest=tests/mochitest/tests/dom/canvas/test/mochitest-subsuite-webgl.ini
for android we use:  --test-manifest=tests/dom/canvas/test/_webgl-conformance.ini

a couple things appear to be missing here:
* we should use --manifest, not --test-manifest (https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/mochitest_options.py?from=mochitest_options.py&case=true#300)
* I suspect the path should include tests/mochitest as well

Lets play with tat a bit more and see if we can see green!
(Assignee)

Comment 91

2 years ago
Pushed with --manifest and full path: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec27ed298ba1
(Reporter)

Comment 92

2 years ago
That didn't work either.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed63119c7707 tries to shed some light on the issue. It introduced some unfortuate failures, but I managed to get some interesting information out of getTestManifest during gl runs.

For Linux:
08:29:47     INFO -  getTestManifest -- options.manifestFile=tests/mochitest/tests/dom/canvas/test/mochitest-subsuite-webgl.ini
08:29:47     INFO -  getTestManifest -- cwd=/builds/slave/test/build
08:29:47     INFO -  getTestManifest -- found file at specified path: /builds/slave/test/build/tests/mochitest/tests/dom/canvas/test/mochitest-subsuite-webgl.ini

For Android:
09:46:35     INFO -  getTestManifest -- options.manifestFile=tests/mochitest/tests/dom/canvas/test/_webgl-conformance.ini
09:46:35     INFO -  getTestManifest -- cwd=/builds/slave/test/build/tests/mochitest
09:46:35     INFO -  getTestManifest -- using masterPath: /builds/slave/test/build/tests/mochitest/tests/mochitest.ini


Note the difference in current working directory.
aha, so if we set --manifest=tests/dom/canvas/tests/_webgl-conformance.ini that would probably solve the problem!
(Assignee)

Comment 94

2 years ago
Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48c03b3f665e

:gbrown, thanks for the insight and helping in this!
(Assignee)

Comment 95

2 years ago
Yay! So the Android 2.3 api9 opt gl jobs have passed in the above try push!

Android 4.0 api11+ opt gl job failed, mostly because I did not change it here: https://dxr.mozilla.org/mozilla-central/source/testing/config/mozharness/android_arm_4_4_config.py#30

I will be pushing again by changing that when I get to my desktop.
(Reporter)

Comment 96

2 years ago
android_arm_4_4_config.py is for a new type of job that is not running yet -- I wouldn't worry about updating that file.

Android 4.0 api11+ opt jobs normally use android_panda_config.py -- but I don't see mochitest-gl configured in that file. I guess it is using https://dxr.mozilla.org/build:mozharness/source/configs/android/android_panda_releng.py#39?
My work in bug 1064002 was focused in Android 2.3.
Let me know if I can help you with the 4.0 jobs.
(Assignee)

Comment 98

2 years ago
Pushed to try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a91c9094fe8

This patch points to my mozharness repo: http://hg.mozilla.org/users/vaibhavmagarwal_gmail.com/vaibhavmagarwal_repo/ so hopefully, the 4.0 jobs should run as expected.
(Assignee)

Comment 99

2 years ago
The android 4.0 gl job is still failing in https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a91c9094fe8

I have made changes to android_panda_config.py and androidx86.py in my repo: http://hg.mozilla.org/users/vaibhavmagarwal_gmail.com/vaibhavmagarwal_repo/rev/94cad7bad7f4 , but it looks like either that is not affecting the changes or it is not picking up the changes. The script_revlink is showing this: http://hg.mozilla.org/users/vaibhavmagarwal_gmail.com/vaibhavmagarwal_repo//rev/7ebb4b339e48 but it should be http://hg.mozilla.org/users/vaibhavmagarwal_gmail.com/vaibhavmagarwal_repo//rev/94cad7bad7f4 . Does anyone knows why this is happening?
(Assignee)

Comment 100

2 years ago
Retriggered the gl jobs after updating the tip of my repo to production: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a91c9094fe8
(Assignee)

Comment 101

2 years ago
Created attachment 8553268 [details] [diff] [review]
mozharness.patch

So the android 4.0 gl job is green on try! Armen, please review this patch :)
Attachment #8553268 - Flags: review?(armenzg)
(Assignee)

Comment 102

2 years ago
Created attachment 8553273 [details] [diff] [review]
gl.patch

:gbrown, please review this patch, it passes on try
Attachment #8553273 - Flags: review?(gbrown)
Comment on attachment 8553268 [details] [diff] [review]
mozharness.patch

Unfortunately, this works on try but it does not mean that it won't burn jobs in some other release tree.

Let's fix this in-tree.

Please add a mochitest-gl category [1][2]:
* by copying the mochitest section
* extend both by adding "--test-manifest=gl.json"

This is a no-op change. Push it to try to make sure nothing is affected.

Once that change lands, we need to check that the mochitest-gl commands on aurora, beta, release and esr31 are the same. This can be determined by looking at the logs of the jobs for Android 4.0 and Android x86 jobs.

If it is all the same, we can then start uplifting everywhere. I can help.

Once all release branches are working properly. We need to do the following:
* Change the two files modified in this patch to do the following:
** "mochitest-gl": [] <-- We stop appending -manifest
** We change the category of "mochitest-gl" jobs to be "mochitest-gl"
*** This will make it start using the category from the in-tree configs

Push to try and make sure it works. You will need to pin to your user repo.

If it works, we can then land it and be done.

The other option is to wait for bug 1110286 to be done (next few weeks) and then this patch would actually not be a problem to land.


[1] http://hg.mozilla.org/mozilla-central/file/default/testing/config/mozharness/android_panda_config.py#l70
[2] http://hg.mozilla.org/mozilla-central/file/default/testing/config/mozharness/android_x86_config.py#l7
Attachment #8553268 - Flags: review?(armenzg) → review-
(Reporter)

Comment 104

2 years ago
Comment on attachment 8553273 [details] [diff] [review]
gl.patch

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

This looks fine, but let's follow through on Armen's advice (comment 103) before considering landing.
Attachment #8553273 - Flags: review?(gbrown)
(Assignee)

Comment 105

2 years ago
Let's do this! Pushed to try by adding mochitest-gl to in-tree configs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9081ed351447
(Assignee)

Comment 106

2 years ago
:armenzg, the push on try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9081ed351447

so does it mean, we can land https://hg.mozilla.org/try/rev/e10d62259032 ?
Flags: needinfo?(armenzg)
Yes, once we review it, however, it is not being used.
It only gets used if on mozharness configs we change the category to be "mochitest-gl" instead of "mochitest.
Flags: needinfo?(armenzg)
<vaibhav1994> armenzg_mtg: if I just remove this line: https://dxr.mozilla.org/build:mozharness/source/configs/android/android_panda_releng.py#39, will it not make it use my in-tree configs?

If you remove it, you will get every "mochitest" parameter plus nothing.
The problem is that we need to read from the new "mochitest-gl" section, however, we don't have that ability for android_panda.py.

Let me describe why they're a bit different.

You can change the category for android 2.3 and android x86 jobs because they both use a new format for the mozharness configs [1], which reads first from the key "test_suite_definitions" and then from the in-gecko configs (from the category you specify)
>    "test_suite_definitions": {
>       "jsreftest": {
>           "category": "reftest",
>           "extra_args": ["../jsreftest/tests/jstests.list",
>               "--extra-profile-file=jsreftest/tests/user.js"]
>       },

Unfortunately, Android 2.3 uses the script android_panda_releng.py [2]
Which chooses the category like this:
> if '%s_options' % suite_category in self.tree_config:
instead of like this:
> suite_category = self.test_suite_definitions[suite_name]["category"]

In your case, suite_category is "mochitest" rather than "mochitest-gl".

You could work on changing android_panda_releng.py to read the new format.

[1] http://hg.mozilla.org/build/mozharness/file/default/scripts/android_emulator_unittest.py#l392
[2] http://hg.mozilla.org/build/mozharness/file/9892549f8789/scripts/android_panda.py#l465
(Assignee)

Comment 109

2 years ago
Armen, thanks for taking the time and explaining in full detail, I now understand the problem fully.
I understand that android_panda_releng.py needs to be converted to the new format, but it involves converting not only mochitests but also reftests, crashtest, jsreftest, robocop, xpcshell, jittest, cppunittests for android jobs. Plus one will also need to change the logic in android_panda.py for the new format. This sounds like a project in itself :)

I can place a *hack* in android_panda.py (somewhere here- [1]) to read from  "mochitest_gl" instead of "mochitest" till the time the full change for new format takes place. Let me know if that works for you in this case. Else, if there is already work being done for converting android_panda_releng.py to new format, I can maybe in that bug and when that gets complete, then we can complete this bug.

[1] http://hg.mozilla.org/build/mozharness/file/9892549f8789/scripts/android_panda.py#l246
Flags: needinfo?(armenzg)
It is a resourceful hack you propose, however, you will have to make sure that the change will allow for riding the trains. In other words, you might use the hack while still running a specific version of Gecko.

I highly suggest waiting for bug 1110286 to be done and this way we won't have to worry about backwards compatibility.
Flags: needinfo?(armenzg)
(Assignee)

Comment 111

2 years ago
After chatting with Armen on IRC, I am trying out my hack to see if it works.

Pushed to treeherder, where it points to my mozharness repo and should use in-tree configs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e30ff3fddf07
(Assignee)

Comment 112

2 years ago
After some debugging and retrying jobs, Android 4.0 API11+ 'gl' job is now working for in-tree configs for try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e30ff3fddf07 . The corresponding patch for my mozharness user repo is - http://hg.mozilla.org/users/vaibhavmagarwal_gmail.com/vaibhavmagarwal_repo/rev/f7d6ffc2abb0

Armen, could you please see the patch of my mozharness repo and let me know if we can move forward with this hack? This hack will move mochitest-gl to in-tree configs and then we can remove gl.json.
Flags: needinfo?(armenzg)
Your mh changes seem correct.

Can you please test these mozharness changes based on mc? All three android platforms. Even if some jobs fail (see advice below).

You also need a separate push with any needed in-tree changes. For instance androidx86's mochitest-gl missing category.

Could you also please attach a patch from your mozharness showing any differences from the official mozharness repo?

Side advise, try not to push to try from your own heads. Always rebase from mc or mi.
The parent of e30ff3fddf07 is e10d62259032 which hides code that you're trying. In this case:
https://hg.mozilla.org/try/diff/e10d62259032/testing/config/mozharness/android_panda_config.py
Pushing from your own heads makes it hard for me (and others) to see which other changes are being tested.

[1] http://hg.mozilla.org/mozilla-central/file/default/testing/config/mozharness/android_x86_config.py
Flags: needinfo?(armenzg)
(Assignee)

Comment 114

2 years ago
Created attachment 8561019 [details] [diff] [review]
moz.patch

try push based on m-i: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ec0d1c75779
(Assignee)

Updated

2 years ago
Depends on: 1131184
(Assignee)

Comment 115

2 years ago
Created attachment 8561529 [details] [diff] [review]
moz.patch

Carrying forward the r+ by :armenzg in bug 1131184. Not to land till bug 1110286 is fixed.
Attachment #8561019 - Attachment is obsolete: true
Attachment #8561529 - Flags: review+
(Assignee)

Comment 116

2 years ago
Created attachment 8561545 [details] [diff] [review]
in_tree.patch

Moving mochitest-gl to in-tree configs. :gbrown, please review this. It is safe to check this in as it is not used currently - will be when we check-in the mozharness changes. related try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ec0d1c75779
Attachment #8561545 - Flags: review?(gbrown)
(Reporter)

Updated

2 years ago
Attachment #8561545 - Flags: review?(gbrown) → review+
(Assignee)

Comment 117

2 years ago
Hey Armen, I see bug 1110286 is fixed. Now can I go ahead with the mozharness fix and move mochitest-gl to in-tree?
Flags: needinfo?(armenzg)
Yes, this is correct.
I completely forgot about this. My apologies.

For a mozharness repo change to be effective will require bumping testing/mozharness/mozharness.json.
Flags: needinfo?(armenzg)

Updated

2 years ago
Whiteboard: [leave open] → [leave open][ateam_harness_work]
(Assignee)

Comment 119

2 years ago
try push to see if the changes are still valid: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43365778ef8d
(Assignee)

Comment 120

2 years ago
Created attachment 8576215 [details] [diff] [review]
mozharness.patch

Armen, the 'gl' jobs for android on the above try push are passing (there are other jobs left to be done just for a check, but the ones which we care about are passing). Please review this patch and since bug 1110286 is fixed, can we land this?
Attachment #8553268 - Attachment is obsolete: true
Attachment #8576215 - Flags: review?(armenzg)
(Assignee)

Comment 121

2 years ago
Created attachment 8576216 [details] [diff] [review]
gl.patch

:gbrown could you please review this. Thanks
Attachment #8553273 - Attachment is obsolete: true
Attachment #8576216 - Flags: review?(gbrown)
(Reporter)

Comment 122

2 years ago
Comment on attachment 8576216 [details] [diff] [review]
gl.patch

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

Please update testing/config/mozharness/android_arm_4_3_config.py as well. It is not being used in any production jobs currently, but will be in use soon.
Attachment #8576216 - Flags: review?(gbrown) → review+
(Assignee)

Comment 123

2 years ago
Created attachment 8576636 [details] [diff] [review]
gl.patch

Updated the patch. Carrying forward the r+
Attachment #8576216 - Attachment is obsolete: true
Attachment #8576636 - Flags: review+
Comment on attachment 8576215 [details] [diff] [review]
mozharness.patch

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

Remember that once landed on mozharnes, grab the revision and we need to land a mozharness.json file on the tree with that revision.
Attachment #8576215 - Flags: review?(armenzg) → review+
(Assignee)

Comment 125

2 years ago
Please checkin the mozharness.patch only for now.
Keywords: checkin-needed
landed the mozharness patch that armen reviewed:
https://hg.mozilla.org/build/mozharness/rev/9e077d6e8a7f
(Assignee)

Comment 127

2 years ago
Created attachment 8576651 [details] [diff] [review]
gl.patch

Armen, :gbrown has reviewed this patch once. I just wanted you to see the mozharness.json file, I have changed the revision to "9e077d6e8a7f". If it is correct, I will check this patch in too. Thanks
Attachment #8576636 - Attachment is obsolete: true
Attachment #8576651 - Flags: review?(armenzg)
Comment on attachment 8576651 [details] [diff] [review]
gl.patch

That's correct.
Once you landed, only that changeset and forward will start using the new mozharness revision.
Sheriffs should be able to back you out if it was needed.
The mozharness change would also need to be backed out if the sheriffs backed out the m-i change.

I don't think a back out would be needed but wanted to give you all the info since this is a new process for most people.
Attachment #8576651 - Flags: review?(armenzg) → review+
(Assignee)

Comment 129

2 years ago
Please check-in gl.patch. Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/404caa140516
the commit message had the wrong bug, it has bug 1083346 instead :(
Keywords: checkin-needed
(Assignee)

Comment 132

2 years ago
Pushed to try for removing android.json, android23.json and androidx86.json: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce5205793d86
mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/d35bfd399151
https://hg.mozilla.org/mozilla-central/rev/879b0f123261
(Assignee)

Comment 135

2 years ago
Created attachment 8577315 [details] [diff] [review]
remove_androidjson_mozharness.patch

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce5205793d86 is green on android jobs till now. I have re-triggered some more. In the meantime, I would like :gbrown and :jmaher to review this mozharness patch so that we can land this *if* the remaining jobs triggers are up to expectation.
Attachment #8561529 - Attachment is obsolete: true
Attachment #8561545 - Attachment is obsolete: true
Attachment #8577315 - Flags: review?(jmaher)
Attachment #8577315 - Flags: review?(gbrown)
Comment on attachment 8577315 [details] [diff] [review]
remove_androidjson_mozharness.patch

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

looks good to me.  I assume once we land this on mozharness, then we need to get it in production and finally update mozilla-inbound.
Attachment #8577315 - Flags: review?(jmaher) → review+
(Reporter)

Updated

2 years ago
Attachment #8577315 - Flags: review?(gbrown) → review+
mozharness patch landed:
https://hg.mozilla.org/build/mozharness/rev/1deaa6e0dc43
(Assignee)

Comment 138

2 years ago
Created attachment 8577388 [details] [diff] [review]
android_final.patch

:jmaher please review this and check it in!
Attachment #8577388 - Flags: review?(jmaher)
Comment on attachment 8577388 [details] [diff] [review]
android_final.patch

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

thanks, this is nice to see.  I would like a followup patch to remove the code in runtests.py/etc. for handling .json manifests.
Attachment #8577388 - Flags: review?(jmaher) → review+
(Assignee)

Comment 140

2 years ago
Yes, I will make another try push for it. Also I think we need to change buildbot configs to remove references to android*.json after this patch gets merged in m-c.
inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51ab04cf3b55

Updated

2 years ago
Whiteboard: [leave open][ateam_harness_work] → [ateam_harness_work]
we need to land lots of this on aurora/beta.  Next week we can build up the patches to uplift as well as figure out what rev of mozharness to uplift- oh joy!

Updated

2 years ago
Depends on: 1143227
https://hg.mozilla.org/mozilla-central/rev/51ab04cf3b55
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(Reporter)

Comment 144

2 years ago
It looks like this has started to cause problems on aurora:

http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-aurora-android-api-11/1426287315/mozilla-aurora_panda_android_test-mochitest-gl-bm102-tests1-panda-build69.txt.gz

17:09:48     INFO - Copy/paste: python -u /builds/panda-0527/test/build/tests/mochitest/runtestsremote.py --deviceIP=10.26.133.70 --xre-path=../hostutils/xre --utility-path=../hostutils/bin --certificate-path=certs --app=org.mozilla.fennec_aurora --console-level=INFO --http-port=30527 --ssl-port=31527 --run-only-tests=android.json --symbols-path=https://ftp-ssl.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-aurora-android-api-11/1426287315/fennec-38.0a2.en-US.android-arm.crashreporter-symbols.zip --quiet --log-raw=/builds/panda-0527/test/build/blobber_upload_dir/mochitest-gl_raw.log --manifest tests/dom/canvas/test/_webgl-conformance.ini
...
17:09:49     INFO -  Usage: runtestsremote.py [options]
17:09:49     INFO -  runtestsremote.py: error: Unable to support both --manifest and --test-manifest/--run-only-tests at the same time
17:09:49    ERROR - Return code: 2
17:09:49    ERROR - No tests run or test summary not found
aurora got mozharness updates which rely on test harness updates as well.  We still need to fix webgl in general in bug 1143218 (this week), so the road to completion has more work.

Tomorrow I could help uplift stuff if needed.
(Assignee)

Updated

2 years ago
Blocks: 1144573
It seems that the Aurora issue was fixed with this:
https://hg.mozilla.org/releases/mozilla-aurora/rev/bdfe1112a65c
You need to log in before you can comment on or make changes to this bug.