1364082 - Improve test about file save in JSON Viewer

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, enhancement, P3)

Description

[Oriol Brufau [:Oriol]]

Attachment: json-save-test.patch

Currently there is only a very basic test about saving in JSON Viewer. It needs to be improved with the new features from bug 1349437 and bug 1327784.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8866788 [details] [diff] [review]
json-save-test.patch

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

Thanks for the patch!

I am seeing following failure when running the test (Win10):

The following tests failed:
13 INFO TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_save_json.js | Exception thrown - Error opening input stream (invalid filename?): chrome://mochitests/content/browser/toolkit/content/tests/browser/common/mockTransfer.js
Buffered messages finished
SUITE-END | took 7s


Honza

Comment 2

[Oriol Brufau [:Oriol]]

Attachment: json-save-test-v2.patch

It seems I needed to reference mockTransfer.js in support-files.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8867695 [details] [diff] [review]
json-save-test-v2.patch

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

Thanks for the patch!

Please read my inline comments.

Honza

::: devtools/client/jsonview/test/browser_jsonview_save_json.js
@@ +13,5 @@
>  
>  let { MockFilePicker } = SpecialPowers;
> +MockFilePicker.init(window);
> +MockFilePicker.returnValue = MockFilePicker.returnOK;
> +var mockTransferCallback, mockTransferRegisterer;

nit style: Define vars independently

var mockTransferCallback;
var mockTransferRegisterer;

@@ +22,5 @@
>  
> +function click(selector) {
> +  return BrowserTestUtils.synthesizeMouseAtCenter(selector, {}, gBrowser.selectedBrowser);
> +}
> +function rightClick(selector) {

Separate function definitions with one empty line.

@@ +44,5 @@
> +        ok(downloadSuccess, "JSON should have been downloaded successfully");
> +        ok(destFile.exists(), "The downloaded file should exist.");
> +        resolve(destFile);
> +      };
> +      void mockTransferCallback; // Needed by eslint

Use `/* eslint-disable no-unused-vars */` at the top of the file instead

@@ +71,5 @@
> +    saveDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0o755);
> +  }
> +  info("Temporary save directory: " + saveDir.path);
> +  return saveDir;
> +}

nit: Insert one empty line here

@@ +101,5 @@
> +  yield new Promise((resolve) => {
> +    info("Register to handle popupshown.");
> +    document.addEventListener("popupshown", function listener(event) {
> +      info("Context menu opened.");
> +      document.removeEventListener("popupshown", listener);

Instead of adding and removing a listener you can use 'once' argument. 
See: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener

Comment 4

[Oriol Brufau [:Oriol]]

Attachment: json-save-test-v3.patch

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8868091 [details] [diff] [review]
json-save-test-v3.patch

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

Looks good, thanks for working on this!

Honza

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/420d6714a827
Improve test about file save in JSON Viewer. r=Honza

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/420d6714a827