926332 - Offer an API in mochitest-chrome and mochitest-browser to safely and easily access data file

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Alexandre Poirot [:ochameau]]

Similarely to bug 894990, we often need to access data file paths, but there is no easy way to do that. Tests most often just copy paste a bunch of code to compute a test data file path. And it requires deep mochitest knowledges to handle nicely the case where tests data files are in a jar, when being run on TBPL!

We should expose an API for getting a nsIFile or an absolute path to a data file living next to a mochitest chrome/browser. This helper would take care of abstracting the jar scenario.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Expose getTestFile to mochitest chrome/browser that returns nsIFile instance for test data files r=jmaher

Comment 2

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

Comment on attachment 816481 [details] [diff] [review]
Expose getTestFile to mochitest chrome/browser that returns nsIFile instance for test data files r=jmaher

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

I would like to see a test case test accessing a file from a subdirectory as well.

::: testing/mochitest/chrome-harness.js
@@ +324,5 @@
> +                      getService(Components.interfaces.nsIFileProtocolHandler);
> +    file = fileHandler.getFileFromURLSpec(parentURI.spec);
> +  }
> +  // Then walk by the given relative path
> +  path.split("/").forEach(function (p) { file.append(p); });

As 'file' is the parent directory of window.location.href, then this will only work if getTestFile is a sibling or descendent of the current test case.  This is indicated in the comment, but it seems as though it could cause confusion.  Can you check the directory structure that you would append here exists?  Of course that would require that we return something else other than file.  Maybe you have a better idea than I would.

Comment 3

[Alexandre Poirot [:ochameau]]

(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 816481 [details] [diff] [review]
> ::: testing/mochitest/chrome-harness.js
> @@ +324,5 @@
> > +                      getService(Components.interfaces.nsIFileProtocolHandler);
> > +    file = fileHandler.getFileFromURLSpec(parentURI.spec);
> > +  }
> > +  // Then walk by the given relative path
> > +  path.split("/").forEach(function (p) { file.append(p); });
> 
> As 'file' is the parent directory of window.location.href, then this will
> only work if getTestFile is a sibling or descendent of the current test
> case.

Right, this function expect a relative path. Is it really usefull to fetch a file from a parent directory? To make it clear, we can enforce the string to always start with "./" and always consider it relative. Also I can add the support for "../" in order to refer to parent folders. We may support absolute paths when the string starts with "/", but I'm not sure we can easily know the root directory and I'm not really convinced we should refer to a test data file via it's absolute path relative to m-c repo.

> This is indicated in the comment, but it seems as though it could
> cause confusion.  Can you check the directory structure that you would
> append here exists?  Of course that would require that we return something
> else other than file.  Maybe you have a better idea than I would.

I don't really get this part of your comment. nsIFile instances can refer to non-existing files. I can add a final check to ensure that the file exists and throw if it doesn't.

Comment 4

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

if we had future support for ../, I would like to see that.  

As for non existent files/directories, we should put a little bit more effort into handling this.  At the very least have some test cases for some mistyped directory names as well as test cases for ./<name> to show that it works.  I would like to see a comment outlining what is expected in the scenario where something is not referenced.  using .exists() is fine as your test case outlines.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 816481 [details] [diff] [review]
Expose getTestFile to mochitest chrome/browser that returns nsIFile instance for test data files r=jmaher

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

nsIFile is evil. Can we use OS.File instead?

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Address review comments

- supports `../` and `./`
- rejects absolute path starting with `/`
- returns a file path instead of nsIFile
- more tests!

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 817126 [details] [diff] [review]
Address review comments

I tried to address the comments as I understood them, but I'm not sure I got all your points, so feel free to point the missing details.


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

Comment 8

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

Comment on attachment 817126 [details] [diff] [review]
Address review comments

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

thanks for the more comprehensive path support and tests, I think this is good enough that we can use this and understand it well in the future.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Rebased to updated browser.ini instead of Makefile.ini

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

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Rebased, but correctly!

I also improved tests logging by printing the error when a promise is rejected in case of OS.file exception.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 817866 [details] [diff] [review]
Rebased, but correctly!

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

Otherwise, I switched to browser.ini (instead of Makefile modification).

And tests are green \o/
https://tbpl.mozilla.org/?tree=Try&rev=4aada61817d0

::: testing/mochitest/chrome/test_chromeGetTestFile.xul
@@ +48,5 @@
> +        })
> +
> +    ]).then(function () {
> +      SimpleTest.finish();
> +    }, console.error);

And there.

::: testing/mochitest/tests/browser/browser_getTestFile.js
@@ +37,5 @@
> +
> +  ]).then(function () {
> +    finish();
> +  }, function (error) {
> +    ok(false, error);

I improved failure logging here.

Comment 12

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

Comment on attachment 817866 [details] [diff] [review]
Rebased, but correctly!

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

my comments are not required for this to land, please view them as extra credit to move our build system to the next level :)

::: testing/mochitest/chrome/Makefile.in
@@ +10,5 @@
>  		test_sanityPluginUtils.html \
>  		test_sanityException.xul \
>  		test_sanityException2.xul \
> +		test_chromeGetTestFile.xul \
> +		test-dir/ \

it would be really cool if you could convert this directory (testing/mochitest/chrome/ to chrome.ini :)

::: testing/mochitest/tests/browser/browser.ini
@@ +11,5 @@
>  [browser_popupNode_check.js]
>  [browser_privileges.js]
>  [browser_sanityException.js]
>  [browser_sanityException2.js]
> +[browser_getTestFile.js]

it would be nice to put the support-files here and reference the test-dir/* as it is only needed for the one test case.  As this is a new concept for us, having it at the top isn't the end of the world, maybe in time that will be preferred.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Tweak browser.ini

Ok, I moved the test-dir, but haven't converted the chrome Makefile.in to chrome.ini
as I would really like to uplift this patch (especially the bug it is blocking).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/62732da6ae3d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-bc timeouts.
https://hg.mozilla.org/integration/fx-team/rev/7cb193c966bf

https://tbpl.mozilla.org/php/getParsedLog.php?id=29268822&tree=Fx-Team

Comment 16

[Matt Brubeck (:mbrubeck)]

The failures were caused by an unrelated patch.  Re-landed:
https://hg.mozilla.org/integration/fx-team/rev/5a315b0b917d

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5a315b0b917d