Closed Bug 1332170 Opened 3 years ago Closed 3 years ago

Make the pdf.js mochitests pass ESLint

Categories

(Firefox :: PDF Viewer, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: RyanVM, Assigned: RyanVM)

Details

Attachments

(1 file)

The in-tree tests aren't maintained upstream and shouldn't be whitelisted like the upstream code is. I've got a patch ready that fixes the found problems in the /test directory and changes the whitelist setting to only include /content.
Bikeshedding welcome on some of the renames done to make the no-shadow rule happy. I tried to follow some of the norms used elsewhere in the tree as those rules were added.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45b114a31614a790fa8d5e935511b870170b0fba

In case you were wondering, here's what issues that were found before I started making any changes.
https://treeherder.mozilla.org/logviewer.html#?job_id=70214046&repo=try
Attachment #8828213 - Flags: review?(ydelendik)
Comment on attachment 8828213 [details] [diff] [review]
fix the pdf.js mochitest ESLint issues

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

::: browser/extensions/pdfjs/test/browser_pdfjs_navigation.js
@@ +195,5 @@
>      });
>    }
>  
>    /**
> +   * The key navigation has to happen in page-fit, otherwise it won"t scroll

Nit: It looks like a slightly too aggressive ' -> " replacement was used here, since a single quote ought to be OK inside of a comment :-)
Darn it, I thought I'd found all of those :P
I wonder if it's worth fixing up the multi-line comments to be consistent across the different tests too.
Comment on attachment 8828213 [details] [diff] [review]
fix the pdf.js mochitest ESLint issues

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

Looks good (w/couple of nits addressed).

::: browser/extensions/pdfjs/test/browser_pdfjs_main.js
@@ +44,3 @@
>  
>          //
>          // Page change from prev/next buttons

nit: change comment to reflect what is asserted below, which is initial page == 1
Attachment #8828213 - Flags: review?(ydelendik) → review+
https://hg.mozilla.org/mozilla-central/rev/fe7445d0ec0a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.