Closed Bug 926332 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → poirot.alex
Attachment #816481 - Flags: review?(jmaher)
Blocks: 920478
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.
Attachment #816481 - Flags: review?(jmaher) → review-
(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.
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 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?
Attached patch Address review comments (obsolete) — Splinter Review
- supports `../` and `./`
- rejects absolute path starting with `/`
- returns a file path instead of nsIFile
- more tests!
Attachment #816481 - Attachment is obsolete: true
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
Attachment #817126 - Flags: review?(jmaher)
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.
Attachment #817126 - Flags: review?(jmaher) → review+
Attached patch Rebased, but correctly! (obsolete) — Splinter Review
I also improved tests logging by printing the error when a promise is rejected in case of OS.file exception.
Attachment #817844 - Attachment is obsolete: true
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.
Attachment #817866 - Flags: review?(jmaher)
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.
Attachment #817866 - Flags: review?(jmaher) → review+
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).
Attachment #817866 - Attachment is obsolete: true
Attachment #818530 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/62732da6ae3d
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
The failures were caused by an unrelated patch.  Re-landed:
https://hg.mozilla.org/integration/fx-team/rev/5a315b0b917d
Status: NEW → ASSIGNED
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5a315b0b917d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
Depends on: 928929
Component: Mochitest Chrome → Mochitest
You need to log in before you can comment on or make changes to this bug.