Closed Bug 1115338 Opened 6 years ago Closed 6 years ago

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


(Firefox Build System :: Mach Core, enhancement)

Not set


(Not tracked)



(Reporter: Gijs, Assigned: kaustabh93)




(1 file, 5 obsolete files)

(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.
OS: Mac OS X → All
Hardware: x86 → All
Attached patch skip.patch (obsolete) — Splinter Review
Comment on attachment 8543168 [details] [diff] [review]

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/
@@ +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-
Attached patch noskip.patch (obsolete) — Splinter Review
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]

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.
Attached patch no-skip.patch (obsolete) — Splinter Review
Attachment #8543517 - Attachment is obsolete: true
Attached patch no-skip.patch (obsolete) — Splinter Review
Attachment #8544120 - Attachment is obsolete: true
Attached patch no-skip.patch (obsolete) — Splinter Review
Attachment #8544123 - Attachment is obsolete: true
Fixed spacing
Comment on attachment 8544123 [details] [diff] [review]

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

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

::: testing/mochitest/
@@ +1632,5 @@
>        for k, v in
>          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
Attached patch no-skip.patchSplinter Review
Attachment #8544123 - Attachment is obsolete: true
Attachment #8544124 - Attachment is obsolete: true
Comment on attachment 8544131 [details] [diff] [review]

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

great stuff, let me push this to try server.
Attachment #8544131 - Flags: review+
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.