Open Bug 958147 Opened 6 years ago Updated 2 years ago

Choose one of run-if or skip-if and get rid of the other

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

REOPENED
mozilla38

People

(Reporter: Ehsan, Assigned: yuj25, Mentored)

References

Details

(Whiteboard: [good first bug][lang=py])

Attachments

(6 files, 16 obsolete files)

14.06 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
martijn.martijn
: review+
Details | Review
21.38 KB, patch
Details | Diff | Splinter Review
5.77 KB, patch
ahal
: review+
Details | Diff | Splinter Review
6.59 KB, patch
anishchandran94
: review+
Details | Diff | Splinter Review
7.06 KB, patch
Details | Diff | Splinter Review
AFAICT |skip-if COND| and |run-if !COND| are equivalent.  There's no reason why we should have two ways of doing this, we should just pick one and remove the other.
Then skip-if it is!  I'm not sure any of them have any inherent advantages over the other one.
I don't think so, it just means you get to stick an extra ! in there for some conditions.
Where does the code responsible for parsing this stuff live?
I want to work on this bug. Please assign this to me.
Assignee: nobody → jaspreetsingh112
Whiteboard: [good first bug][lang=py][mentor=ted.mielczarek]
Attached patch choose.patch (obsolete) — Splinter Review
I'll make the required changes in http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestdestiny/manifestparser/manifestparser.py#1034 once this runs clean
Attachment #8378103 - Flags: review?(ted)
Comment on attachment 8378103 [details] [diff] [review]
choose.patch

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

Thanks for the patch! This looks great, I just have one thing I'd like changed. Also, we should definitely run this by the try server.

::: testing/mozbase/manifestdestiny/tests/filter-example.ini
@@ +1,4 @@
>  # illustrate test filters based on various categories
>  
>  [windowstest]
> +skip-if = os != 'win'

Heh, this is a unit test for manifestdestiny. How about you leave this out until we remove run-if support from the manifestparser?
Attachment #8378103 - Flags: review?(ted) → review+
Attached patch choose2.patch (obsolete) — Splinter Review
updated with the requested change
Attachment #8378103 - Attachment is obsolete: true
Attachment #8378373 - Flags: review?(ted)
Mentor: ted
Whiteboard: [good first bug][lang=py][mentor=ted.mielczarek] → [good first bug][lang=py]
ted, did we land this?
Flags: needinfo?(ted)
I don't think it landed, no.
Flags: needinfo?(ted)
Ted, can you verify this patch solves this bug and get it landed?
Flags: needinfo?(ted)
repinging :ted on here, we are 10 weeks out with crickets.
Flags: needinfo?(ted)
pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7f50b156cbb

If this goes well, we can land this and assert if we have remaining run-if statements.
https://hg.mozilla.org/mozilla-central/rev/fbfffe99ab60
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Joel: thanks for getting this landed!  	
Jaspreet: thanks for the patch!
hmm, there are a few more instances now:
https://dxr.mozilla.org/mozilla-central/search?q=run-if&case=true&redirect=true

maybe we could do another round here, get it in and remove the run-if tag from manifestparser?
You can just have blanket-r=me to fix those. If you want to get a patch up to remove run-if from manifestparser I can review that in advance and you can land them together.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
@Ted Mielczarek I want to work on this bug.Please assign it to me.
Lalit: Hi there. I'm sorry, but I think someone else is already working on the remaining parts of this bug.
yah thanks Ted. Lalit : sorry man ! I am already working on it
Attached patch first.patch (obsolete) — Splinter Review
I have made a patch to prohibit the further use of run-if´s and also replaced the already existing run-if´s with skip-if
Comment on attachment 8558010 [details] [diff] [review]
first.patch

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

on the items I mention I am not sure, no need to change things, the rest should be adjusted and I will run it on try server.

::: browser/components/uitour/test/browser.ini
@@ +21,5 @@
>  skip-if = e10s # Bug 941428 - UITour.jsm not e10s friendly.
>  [browser_UITour_loop.js]
>  skip-if = e10s # Bug 941428 - UITour.jsm not e10s friendly.
>  [browser_UITour_modalDialog.js]
> +skip-if = os!= "mac" && e10s # modal dialog disabling only working on OS X.Bug 941428 - UITour.jsm not e10s friendly

1) keep the spacing nicely: os != "mac"
2) || e10s

::: dom/ipc/tests/mochitest.ini
@@ +13,2 @@
>  [test_CrashService_crash.html]
> +skip-if = !(crashreporter && !e10s && (toolkit == 'gtk2' || toolkit == 'gtk3' || toolkit == 'cocoa' || toolkit == 'windows') && (buildapp != 'b2g' || toolkit == 'gonk'))

I am not sure if this will work.  In fact I am not sure what the original statement means.

(toolkit == 'gtk2' || toolkit == 'gtk3' || toolkit == 'cocoa' || toolkit == 'windows') - I believe this is linux/windows/mac
(buildapp != 'b2g' || toolkit == 'gonk') - I believe this is desktop b2g

so really we want to skip-if: !crashreporter && e10s && (buildapp != 'b2g' || android)

I am not sure.

::: image/test/mochitest/mochitest.ini
@@ +92,5 @@
>  skip-if = toolkit == "gonk" #Bug 997034 - canvas.toDataURL() often causes lost connection to device.
>  [test_error_events.html]
>  [test_short_gif_header.html]
>  [test_image_buffer_limit.html]
> +Firefox OS currently.

we have a random line here.  Hmm, we should leave this in since it had metadata about the test, but lets put it as:
#skip-if = toolkit != "gonk" #<original comment>

::: memory/replace/dmd/test/xpcshell.ini
@@ +28,5 @@
>  
>  # Bug 1077230 explains why this test is disabled on Mac 10.6.
>  [test_dmd.js]
>  dmd = true
> +skip-if = !(os == 'linux' || os == 'mac' && os_version != '10.6' || os == 'win')

I am not sure if this is correct.

::: testing/mozbase/docs/manifestparser.rst
@@ +282,5 @@
>    test is disabled
>  - tags : keys and values to filter on (e.g. `os='linux'`)
>  
>  `active_tests` looks for tests with `skip-if`
> +.  If the condition is or is not fulfilled,

this doesn't look correct, could you pull the period to the previous like and format it similarly.

@@ +287,4 @@
>  respectively, the test is marked as disabled.  For instance, if you
>  pass `**dict(os='linux')` as `**tags`, if a test contains a line
>  `skip-if = os == 'linux'` this test will be disabled, or
> +.  It

same here.

::: testing/mozbase/manifestparser/manifestparser/manifestparser.py
@@ +705,5 @@
>  
>      def filter(self, values, tests):
>          """
>          filter on a specific list tag, e.g.:
> +        skip-if = os != win linux

just remove this line
Attachment #8558010 - Flags: review-
Assignee: jaspreetsingh112 → anishchandran94
Attached patch first.patch (obsolete) — Splinter Review
Improved first patch based on comments
Attachment #8558010 - Attachment is obsolete: true
Comment on attachment 8558061 [details] [diff] [review]
first.patch

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

4 things below, please do fix these up and I think we will be ready to go.

::: image/test/mochitest/mochitest.ini
@@ +93,4 @@
>  [test_error_events.html]
>  [test_short_gif_header.html]
>  [test_image_buffer_limit.html]
> +Firefox OS currently.

please fix this line, this won't build or run at all, lets convert it to a skip-if and comment it out with the same comment.

::: memory/replace/dmd/test/xpcshell.ini
@@ +28,5 @@
>  
>  # Bug 1077230 explains why this test is disabled on Mac 10.6.
>  [test_dmd.js]
>  dmd = true
> +skip-if = os != 'linux' && (os == 'mac' && os_version == '10.6') && os != 'win')

this has a trailing ')', that won't work.

Also I think this should be:
skip-if = !(linux && win) || mac=10.6

::: testing/mozbase/docs/manifestparser.rst
@@ +281,5 @@
>    test's metadata will be present and will be set to the reason that a
>    test is disabled
>  - tags : keys and values to filter on (e.g. `os='linux'`)
>  
> +`active_tests` looks for tests with `skip-if`.If the condition is or is not fulfilled,respectively, the test is marked as disabled.  For instance, if you pass `**dict(os='linux')` as `**tags`, if a test contains a line`skip-if = os == 'linux'` this test will be disabled, or

two spaces after a .

'skip-if'.  If the...

@@ +282,5 @@
>    test is disabled
>  - tags : keys and values to filter on (e.g. `os='linux'`)
>  
> +`active_tests` looks for tests with `skip-if`.If the condition is or is not fulfilled,respectively, the test is marked as disabled.  For instance, if you pass `**dict(os='linux')` as `**tags`, if a test contains a line`skip-if = os == 'linux'` this test will be disabled, or
> +.It is up to the harness to pass in tags appropriate to its usage.

this is messed up, it should be more like:
if a test contains a line `skip-if = os = 'linux'` this test will be disabled.  It is up ....
Attachment #8558061 - Flags: review-
Attached patch first.patch (obsolete) — Splinter Review
Altered based on the comments
Attachment #8558061 - Attachment is obsolete: true
Comment on attachment 8558100 [details] [diff] [review]
first.patch

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

ah, 2 small issues.

When you get these fixed, do a build and verify it completes.  Then do a simple sanity test run with mach.

::: image/test/mochitest/mochitest.ini
@@ +88,5 @@
>  [test_bug89419-2.html]
>  skip-if = (toolkit == 'android' && processor == 'x86') #x86 only
>  [test_animation_operators.html]
>  [test_drawDiscardedImage.html]
> +skip-if = toolkit != "gonk" #Image buffer limit is only set for Firefox OS currently.

the comment here is missing- the skip-if is correct.

@@ +93,4 @@
>  [test_error_events.html]
>  [test_short_gif_header.html]
>  [test_image_buffer_limit.html]
>  #run-if = toolkit == "gonk" #Image buffer limit is only set for Firefox OS currently.

leave this commented out and change it to skip-if = toolkit != "gonk"

::: memory/replace/dmd/test/xpcshell.ini
@@ +28,5 @@
>  
>  # Bug 1077230 explains why this test is disabled on Mac 10.6.
>  [test_dmd.js]
>  dmd = true
> +skip-if = !(linux && win) || mac=10.6

this won't work, we need proper syntax here:
skip-if !(os=='linux' || os=='win') || (os=='mac' && os_version=='10.6')
Attachment #8558100 - Flags: review-
Attached patch first.patch (obsolete) — Splinter Review
Attachment #8558100 - Attachment is obsolete: true
Attached patch first.patch (obsolete) — Splinter Review
Attachment #8558551 - Attachment is obsolete: true
Comment on attachment 8558616 [details] [diff] [review]
first.patch

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

great, let me test this on try server
Attachment #8558616 - Flags: review+
on try server there are two error I see of concern:
* b2g desktop 'a' job: https://treeherder.mozilla.org/logviewer.html#?job_id=4687164&repo=try
** found a run-if in '/builds/slave/test/gaia/tests/python/gaia-ui-tests/gaiatest/tests/accessibility/manifest.ini'
** this is in a different repo and located here: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/accessibility/ftu/manifest.ini

* mulet linux 64 '1' job: https://treeherder.mozilla.org/logviewer.html#?job_id=4687166&repo=try
** 03:21:18 INFO - 1010 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_hasFeature.html | Endpoint Navigator.getMobileIdAssertion resolved with the correct value. If this is failing because you're changing how an API is exposed, you must contact the Marketplace team to let them know about the change. - got true, strictly expected undefined
** this manifest: https://dxr.mozilla.org/mozilla-central/source/dom/base/test/mochitest.ini?from=dom/base/test/mochitest.ini&case=true#1
** two run-if conditions switched to skip-if- not sure if this is related to an existing failure or the changes.
** we don't seem to run this test on any of the branches (inbound, central, b2g-inbound), so I have no idea if this passes normally

The first issue is something we will need to fix prior to landing this, :davehunt, what is the process for changing a gaia-ui-test?  Do we submit a pull request, file a bug, etc?
Flags: needinfo?(dave.hunt)
(In reply to Joel Maher (:jmaher) from comment #37)
> The first issue is something we will need to fix prior to landing this,
> :davehunt, what is the process for changing a gaia-ui-test?  Do we submit a
> pull request, file a bug, etc?

Please provide a pull request and attach to a bug. This bug will be fine, you can ask :Bebe for review.
Flags: needinfo?(dave.hunt)
Comment on attachment 8569362 [details] [review]
Pull Request for the change in run-if in the files in gaia (checked in)

Looks good, the test run also was green on treeherder try.
Attachment #8569362 - Flags: review?(martijn.martijn) → review+
(In reply to Martijn Wargers [:mwargers] (QA) from comment #41)
> Comment on attachment 8569362 [details] [review]
> Pull Request for the change in run-if in the files in gaia
> 
> Looks good, the test run also was green on treeherder try.

Merged in https://github.com/mozilla-b2g/gaia/commit/2e92c9bf543f5401c65402a775c9adc388f45673
Attachment #8569362 - Attachment description: Pull Request for the change in run-if in the files in gaia → Pull Request for the change in run-if in the files in gaia (checked in)
Attachment #8555855 - Attachment description: s/run-if/skip-if/ updated original patch for bitrot (2.1) → s/run-if/skip-if/ updated original patch for bitrot (2.1) (checked in)
Attached patch firstpatch.diff (obsolete) — Splinter Review
Attachment #8558616 - Attachment is obsolete: true
Attachment #8570581 - Attachment is obsolete: true
I think the last patch ( https://bugzilla.mozilla.org/attachment.cgi?id=8570895 ) can be checked in, right Joel? The treeherder try run seems fine to me and you already reviewed this patch.
Flags: needinfo?(jmaher)
yes, this is ready for checkin!
Flags: needinfo?(jmaher)
Keywords: checkin-needed
Once this lands, don't forget to remove the run_if filter:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/manifestparser/filters.py#34
Whiteboard: [good first bug][lang=py] → [good first bug][lang=py][leave open]
Attachment #8570895 - Attachment description: run.patch → run.patch (checked in)
(In reply to Andrew Halberstadt [:ahal] from comment #49)
> Once this lands, don't forget to remove the run_if filter:
> https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/
> manifestparser/manifestparser/filters.py#34

Anish, could you perhaps do that? After that is done, I think this bug can be closed.
Flags: needinfo?(anishchandran94)
yah ! sure mwargers I will edit it and make a patch soon !
Flags: needinfo?(anishchandran94)
Attached patch final.patch (obsolete) — Splinter Review
Time to close it :)
Attachment #8572550 - Flags: review?(ahalberstadt)
Comment on attachment 8572550 [details] [diff] [review]
final.patch

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

Thanks for the patch, though there's some stuff you're missing. You'll also need to:
* remove it from DEFAULT_FILTERS in that file
* update the docstring for DEFAULT_FILTERS
* remove the associated test: https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/manifestparser/tests/test_filters.py#105
Attachment #8572550 - Flags: review?(ahalberstadt) → review-
Attached patch final.patch (obsolete) — Splinter Review
Attachment #8572550 - Attachment is obsolete: true
Attachment #8572719 - Flags: review?(ahalberstadt)
Comment on attachment 8572719 [details] [diff] [review]
final.patch

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

Thanks! Please address the two comments below and upload a new patch that includes the original change as well.

::: testing/mozbase/manifestparser/tests/test_filters.py
@@ -91,4 @@
>          { "name": "test3", "fail-if": "foo == 'bar'" },
>          { "name": "test4", "disabled": "some reason" },
>          { "name": "test5", "subsuite": "baz" },
>          { "name": "test6", "subsuite": "baz,foo == 'bar'" })

Please rename the test numbers

@@ -103,5 @@
>          self.assertNotIn(self.tests[1], tests)
>  
> -    def test_run_if(self):
> -        tests = deepcopy(self.tests)
> -        tests = list(run_if(tests, {}))

You'll also need to remove the import.
Attachment #8572719 - Flags: review?(ahalberstadt)
Attached patch final.patch (obsolete) — Splinter Review
Attachment #8572719 - Attachment is obsolete: true
Attachment #8572755 - Flags: review?(ahalberstadt)
Comment on attachment 8572755 [details] [diff] [review]
final.patch

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

Thanks, but this patch doesn't have the original change that removes the run_if function, could you combine this with your first patch so that they can be landed together?
Attachment #8572755 - Flags: review?(ahalberstadt) → review+
Attached patch final.patch (obsolete) — Splinter Review
Attachment #8572755 - Attachment is obsolete: true
Attachment #8572812 - Flags: review?(ahalberstadt)
Attached patch final.patch (obsolete) — Splinter Review
Attachment #8572812 - Attachment is obsolete: true
Attachment #8572812 - Flags: review?(ahalberstadt)
Attachment #8572817 - Flags: review?(ahalberstadt)
Comment on attachment 8572817 [details] [diff] [review]
final.patch

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

Thanks for your contribution!
Attachment #8572817 - Flags: review?(ahalberstadt) → review+
Attached patch final.patch (obsolete) — Splinter Review
Attachment #8572817 - Attachment is obsolete: true
Attachment #8572824 - Flags: review?(ahalberstadt)
Comment on attachment 8572824 [details] [diff] [review]
final.patch

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

Thanks for updating the patch info, for future no need to ask for a new review, you can just set it to a + yourself :)
Attachment #8572824 - Flags: review?(ahalberstadt) → review+
This just modifies the commit message.
Attachment #8572824 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [good first bug][lang=py][leave open] → [good first bug][lang=py]
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7245538&repo=mozilla-inbound
Flags: needinfo?(anishchandran94)
Attached patch final.patch (obsolete) — Splinter Review
Flags: needinfo?(anishchandran94)
Attachment #8573234 - Flags: review?(jmaher)
Attachment #8573234 - Flags: review?(jmaher) → review+
Attached patch final.patchSplinter Review
Attachment #8573234 - Attachment is obsolete: true
Attachment #8573245 - Flags: review+
Depends on: 1172568
> ::: memory/replace/dmd/test/xpcshell.ini
> @@ +28,5 @@
> >  
> >  # Bug 1077230 explains why this test is disabled on Mac 10.6.
> >  [test_dmd.js]
> >  dmd = true
> > +skip-if = !(linux && win) || mac=10.6
> 
> this won't work, we need proper syntax here:
> skip-if !(os=='linux' || os=='win') || (os=='mac' && os_version=='10.6')

That's an incorrect rewrite :(  See bug 1172568.
Attachment #8649456 - Flags: review?(jmaher)
thanks for updating the patch Stanley!  I have pushed to try and in 4 hours or so (i.e. tomorrow) we will see the results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a799a9bf7cca

The results will determine if we need to fix other areas that have changed since the patch was originally created.
Comment on attachment 8649456 [details] [diff] [review]
final.patch after bitrot

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

cancelling the review, on Try there are a lot of consistent failures, I believe most of these are due to manifests which have run-if conditions.  Those will need to be converted to skip-if either as part of this patch or in a secondary patch which we land first.
Attachment #8649456 - Flags: review?(jmaher)
Hello
My name is Jaewoong Yu and I am a student at Coventry University.  
I was wondering if I could ask you something
Is it possible to get permission to fix the bugs?
Because this is my coursework for my subject which is 389COM open-source-development.
Jaewoong Yu: I have assigned the bug to you. I think this is fine since it has been two years since Anish tried to fix it. I suggest using the existing patches as a starting point, but you will probably need to do additional work to account for the changes since then.

If you haven't seen it already, 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction has lots of helpful information for new contributors. Good luck!
Assignee: anishchandran94 → yuj25
Hello
I am Zhangdailin from Coventry University
Could you allow me to fix the bugs for my coursework?
I am doing Open-source-development.
Thank you
(In reply to Jaewoongyu from comment #75)
> Hello
> My name is Jaewoong Yu and I am a student at Coventry University.  
> I was wondering if I could ask you something
> Is it possible to get permission to fix the bugs?
> Because this is my coursework for my subject which is 389COM
> open-source-development.

Hello
I actually couldn't find the bugs. Could you please give me the link to go there?
Thank you
Jaewoong Yu: this page is the bug report for one bug, which has bug number 958147. There are some old patches attached in the "Attachments" section near the top of the page. One or more of these might be a good starting place, but they are likely to be out-of-date and needing changes. I recommend you read through all the comments to get an understanding of the problem to be solved, and prior attempts to solve them.
(In reply to Nicholas Nethercote [:njn] from comment #79)
> Jaewoong Yu: this page is the bug report for one bug, which has bug number
> 958147. There are some old patches attached in the "Attachments" section
> near the top of the page. One or more of these might be a good starting
> place, but they are likely to be out-of-date and needing changes. I
> recommend you read through all the comments to get an understanding of the
> problem to be solved, and prior attempts to solve them.

could you assigned me a bug????
Dailin Zhang: I assigned this bug to Jaewoong Yu because they asked first.

To find another suitable bug, I suggest you read https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Introduction, particularly the "Step 2 - Find something to work on" section.
I have read all comments but I am still not 100% sure which file of bugs I should fix.

dom/canvas/test/webgl-conf/generate-wrappers-and-manifest.py can I fix this bug ? I have just found it from the link on the comments.
Could you please reply to me ??

Thank you
can i work on this issue
You need to log in before you can comment on or make changes to this bug.