If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

./mach mochitest-browser path/to/individual/test should always run test (despite skip-if/run-if conditions in manifest)

RESOLVED FIXED in mozilla37

Status

()

Core
mach
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Gijs, Assigned: Kaustabh Datta Choudhury)

Tracking

Trunk
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

3 years ago
(In reply to Mark Hammond [:markh] from bug 1107776 comment #2)
> * As a different issue and as Ted mentions, it might be useful to ignore
> such conditionals when a single test is specified by the user.  However,
> this will be impossible to do generically as some tests are filtered at
> build time, so not all disabled tests are actually available to run.  But it
> *is* possible to fix for e10s specifically as these aren't filtered at build
> time.

(In reply to Ted Mielczarek [:ted.mielczarek] from bug 1107776 comment #3)
> We no longer filter any tests at build time, so that should not be a
> problem.
(Reporter)

Updated

3 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

3 years ago
Created attachment 8543168 [details] [diff] [review]
skip.patch
Comment on attachment 8543168 [details] [diff] [review]
skip.patch

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

This is a fine approach as we just want to run the loop once, there is no need for looping or bisection logic if we are running a single test case.  Please address the comments made and upload again.

::: testing/mochitest/runtests.py
@@ +1627,5 @@
> +       testPath.endswith('.js'):
> +        return True
> +    else:
> +        return False
> +    

a few things here:
1) extra whitespace on line 1631
2) this file is 2 space indentation, please ensure the whole function is 2 space indented
3) change isSingle to isSingleTest

@@ +1756,2 @@
>  
>      if not options.runByDir:

I would do the logic on this if condition instead:
if not options.runByDir or self.isSingleTest(options):
Attachment #8543168 - Flags: review-
(Assignee)

Comment 3

3 years ago
Created attachment 8543517 [details] [diff] [review]
noskip.patch

The isSingleTest is fixed and now yields proper results. However using the condition ' if not options.runByDir: ' led to the execution of multiple tests even if only one was invoked. So I am sticking to making runbydir dependent on whether a single test is invoked or a collection of tests.

Please review. Thank You!
Attachment #8543168 - Attachment is obsolete: true
Comment on attachment 8543517 [details] [diff] [review]
noskip.patch

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

I really don't like this approach.  It seems quite hacky and for someone reading this it doesn't make sense.  As I mentioned in a previous comment lets keep runbydir = true|false.  We could change the if condition that assigns it though.

I highly suspect we could fix something in getTestManifest or getActiveTests to solve this.
(Assignee)

Comment 5

3 years ago
Created attachment 8544120 [details] [diff] [review]
no-skip.patch
Attachment #8543517 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
Created attachment 8544123 [details] [diff] [review]
no-skip.patch
Attachment #8544120 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8544124 [details] [diff] [review]
no-skip.patch
Attachment #8544123 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Fixed spacing
Comment on attachment 8544123 [details] [diff] [review]
no-skip.patch

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

a few things on a list here to address prior to reviewing.

::: testing/mochitest/runtests.py
@@ +1632,5 @@
>        for k, v in mozinfo.info.items():
>          if isinstance(k, unicode):
>            k = k.encode('ascii')
>          info[k] = v
> +   

nit: blank line with whitespace.

@@ +1644,5 @@
>            # In the case where we have a single file, we don't want to filter based on options such as subsuite.
>            tests = manifest.active_tests(disabled=disabled, options=None, **info)
>            for test in tests:
>                if 'disabled' in test:
> +                del test['disabled']

I see you have this at 2 space indentation like the rest of this file is, could you just the rest of the code around this code block also? :)  It appears we have allowed some 4 space indentation in on accident.

@@ +1650,5 @@
>        else:
>            tests = manifest.active_tests(disabled=disabled, options=options, **info)
> +          if len(tests) == 0:
> +            tests = manifest.active_tests(disabled=True, options=options, **info)
> +       

Nit: you have a blank like with whitespace, make it a blank line

@@ +1658,2 @@
>      for test in tests:
> +      if isSingleTest == 1 and 'disabled' in test:

you could just do len(tests) and avoid isSingleTest.

@@ +1748,5 @@
>      """ Prepare, configure, run tests and cleanup """
>  
>      self.setTestRoot(options)
>  
> +    manifest = self.getTestManifest(options)

I don't believe this is necessary.  If so, please explain.
Attachment #8544123 - Attachment is obsolete: false
(Assignee)

Comment 10

3 years ago
Created attachment 8544131 [details] [diff] [review]
no-skip.patch
Attachment #8544123 - Attachment is obsolete: true
Attachment #8544124 - Attachment is obsolete: true
Comment on attachment 8544131 [details] [diff] [review]
no-skip.patch

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

great stuff, let me push this to try server.
Attachment #8544131 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=65c8a172f86b
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3595d450fac1
Assignee: nobody → kaustabh93
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3595d450fac1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Reporter)

Comment 15

3 years ago
Did you test this? With which test suites?

I just tried running a disabled-in-e10s mochitest-browser test with ./mach mochitest-browser --e10s , and I get:

4 INFO TEST-UNEXPECTED-FAIL | browser-test.js | No tests to run. Did you pass an invalid --test-path?

so this doesn't seem fixed to me. Want me to file a new bug, or reopen this one, or...?
Flags: needinfo?(jmaher)
hmm, I was looking at this for subsuite issues, not skip-if related stuff.  I am fine reopening this or opening another bug.  That kind of a fix would need to go into the harness where we parse the ini files and then ignore the disabled attribute (or don't pass in mozinfo) if we have a single test case.
Flags: needinfo?(jmaher)
You need to log in before you can comment on or make changes to this bug.