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

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[:Gijs (he/him)]

(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.

Comment 1

[Kaustabh Datta Choudhury]

Attachment: skip.patch

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 3

[Kaustabh Datta Choudhury]

Attachment: 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!

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 5

[Kaustabh Datta Choudhury]

Attachment: no-skip.patch

Comment 6

[Kaustabh Datta Choudhury]

Attachment: no-skip.patch

Comment 7

[Kaustabh Datta Choudhury]

Attachment: no-skip.patch

Comment 8

[Kaustabh Datta Choudhury]

Fixed spacing

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 10

[Kaustabh Datta Choudhury]

Attachment: no-skip.patch

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

https://tbpl.mozilla.org/?tree=Try&rev=65c8a172f86b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3595d450fac1

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3595d450fac1

Comment 15

[:Gijs (he/him)]

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...?

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

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.