Closed Bug 875766 Opened 9 years ago Closed 7 years ago

Add automated test for navigation through the document

Categories

(Firefox :: PDF Viewer, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
firefox36 --- affected

People

(Reporter: mihaelav, Assigned: cosmin-malutan)

Details

(Whiteboard: [pdfjs-c-testing])

Attachments

(1 file, 3 obsolete files)

Add automated test for  navigation through the document.
Manual test: https://moztrap.mozilla.org/manage/case/367/

Here are the steps:
1. Open a PDF which has more pages
2. Navigate the document using the Previous and Next buttons
3. Scroll the document using the scroll bar
4. Navigate the document by entering a the page number in the corresponding text box
5. Navigate the PDF using the thumbnail view of the document
6. Navigate the PDF using the outline of the document

Expected result: Navigation works fine in either case
I wonder if step 3 (navigation using the scroll bar) can be simulated in any way.
Whiteboard: [pdfjs-c-testing]
Assignee: mihaela.velimiroviciu → cosmin.malutan
Attached patch patch v1.0 (obsolete) — Splinter Review
What the test dose:
* Opens a pdf in a new tab.
* Outline items are added asynchronously so it waits for them.
* Then it uses each case specified in comment 1, to navigate trough document, wait for page to change, checks that the current page is the expected one.

I used promises to wait for page changes, when they are resolved I move on to the next test action.
Attachment #8505349 - Flags: review?(ydelendik)
Comment on attachment 8505349 [details] [diff] [review]
patch v1.0

Flagging other developers for review, maybe ydelendik is not available.
Attachment #8505349 - Flags: review?(dtownsend+bugmail)
Attachment #8505349 - Flags: review?(bdahl)
Comment on attachment 8505349 [details] [diff] [review]
patch v1.0

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

Almost there, just a couple of things I'd like to see fixed for this to land.

::: browser/extensions/pdfjs/test/browser_pdfjs_navigation.js
@@ +115,5 @@
> +}
> +
> +function runTests(document, window, tab, finish) {
> +  // Focus the tab
> +  tab.click();

Just set gBrowser.selectedBrowser = newTabBrowser above after creating the tab.

@@ +136,5 @@
> + * @param window
> + * @param test
> + * @param callback
> + */
> +function goToPage(document, window, test, endCallback) {

I think it would make more sense to name this runNextTest and the first statement should be "var test = TESTS.shift()" rather than having to pass the test in each time.

@@ +145,5 @@
> +  window.addEventListener('pagechange', function pageChange() {
> +    if (pageNumber.value == test.expectedPage) {
> +      window.removeEventListener('pagechange', pageChange);
> +      deferred.resolve(pageNumber.value);
> +    }

If it goes to the wrong page then the promise never resolves and the test just times out. Seems like it would be better to always resolve.

@@ +170,5 @@
> +      goToPage(document, window, nextTest, endCallback);
> +    else
> +      endCallback();
> +  }, function () {
> +    ok(false, "Test '" + test.message + "' failed with timeout.");

I don't see anything in the test rejecting the promise so I don't know how this would ever be called.
Attachment #8505349 - Flags: review?(dtownsend+bugmail) → review-
Attached patch patch v2.0 (obsolete) — Splinter Review
Thanks for review, it made perfect sense. 

(In reply to Dave Townsend [:mossop] from comment #4)
> > +  window.addEventListener('pagechange', function pageChange() {
> > +    if (pageNumber.value == test.expectedPage) {
> > +      window.removeEventListener('pagechange', pageChange);
> > +      deferred.resolve(pageNumber.value);
> > +    }
> If it goes to the wrong page then the promise never resolves and the test
> just times out. Seems like it would be better to always resolve.
This is similar to mousemove event, it triggers while the page scrolls and I have to be sure we are in the right page, anyway I added a timeout and reject the promise if it fails.

> @@ +170,5 @@
> > +      goToPage(document, window, nextTest, endCallback);
> > +    else
> > +      endCallback();
> > +  }, function () {
> > +    ok(false, "Test '" + test.message + "' failed with timeout.");
> 
> I don't see anything in the test rejecting the promise so I don't know how
> this would ever be called.
I fixed this I added a timeout and reject the promise and call finish so it won't hang.
Attachment #8505349 - Attachment is obsolete: true
Attachment #8505349 - Flags: review?(ydelendik)
Attachment #8505349 - Flags: review?(bdahl)
Attachment #8508484 - Flags: review?(dtownsend+bugmail)
(In reply to Cosmin Malutan from comment #5)
> Created attachment 8508484 [details] [diff] [review]
> patch v2.0
> 
> Thanks for review, it made perfect sense. 
> 
> (In reply to Dave Townsend [:mossop] from comment #4)
> > > +  window.addEventListener('pagechange', function pageChange() {
> > > +    if (pageNumber.value == test.expectedPage) {
> > > +      window.removeEventListener('pagechange', pageChange);
> > > +      deferred.resolve(pageNumber.value);
> > > +    }
> > If it goes to the wrong page then the promise never resolves and the test
> > just times out. Seems like it would be better to always resolve.
> This is similar to mousemove event, it triggers while the page scrolls and I
> have to be sure we are in the right page, anyway I added a timeout and
> reject the promise if it fails.

Do you mean that this fires for every page as it scrolls through?
Flags: needinfo?(cosmin.malutan)
Attached patch patch v2.0 (obsolete) — Splinter Review
(In reply to Dave Townsend [:mossop] from comment #6)
> > > If it goes to the wrong page then the promise never resolves and the test
> > > just times out. Seems like it would be better to always resolve.
> > This is similar to mousemove event, it triggers while the page scrolls and I
> > have to be sure we are in the right page, anyway I added a timeout and
> > reject the promise if it fails.
> 
> Do you mean that this fires for every page as it scrolls through?
Yes, it fires multiple times while the page scrolls/animates.
Also I just noticed I attached the same patch, sorry for that, here is the one with the changes you have requested.
Attachment #8508484 - Attachment is obsolete: true
Attachment #8508484 - Flags: review?(dtownsend+bugmail)
Flags: needinfo?(cosmin.malutan)
Attachment #8509236 - Flags: review?(dtownsend+bugmail)
Status: NEW → ASSIGNED
(In reply to Cosmin Malutan from comment #7)
> Created attachment 8509236 [details] [diff] [review]
> patch v2.0
> 
> (In reply to Dave Townsend [:mossop] from comment #6)
> > > > If it goes to the wrong page then the promise never resolves and the test
> > > > just times out. Seems like it would be better to always resolve.
> > > This is similar to mousemove event, it triggers while the page scrolls and I
> > > have to be sure we are in the right page, anyway I added a timeout and
> > > reject the promise if it fails.
> > 
> > Do you mean that this fires for every page as it scrolls through?
> Yes, it fires multiple times while the page scrolls/animates.

So if I understand correctly if I perform an action that causes us to jump from page 1 to page 4 the pagechange event fires for pages 2, 3 and 4. The problem is that if the test is expecting it to end on page 3 then it will see the pagechange for page 3, continue and record the test as passed ignoring the fact that it later moved to page 4.
(In reply to Dave Townsend [:mossop] from comment #8)
> (In reply to Cosmin Malutan from comment #7)
> > Created attachment 8509236 [details] [diff] [review]
> > patch v2.0
> > 
> > (In reply to Dave Townsend [:mossop] from comment #6)
> > > > > If it goes to the wrong page then the promise never resolves and the test
> > > > > just times out. Seems like it would be better to always resolve.
> > > > This is similar to mousemove event, it triggers while the page scrolls and I
> > > > have to be sure we are in the right page, anyway I added a timeout and
> > > > reject the promise if it fails.
> > > 
> > > Do you mean that this fires for every page as it scrolls through?
> > Yes, it fires multiple times while the page scrolls/animates.
> 
> So if I understand correctly if I perform an action that causes us to jump
> from page 1 to page 4 the pagechange event fires for pages 2, 3 and 4. The
> problem is that if the test is expecting it to end on page 3 then it will
> see the pagechange for page 3, continue and record the test as passed
> ignoring the fact that it later moved to page 4.

That's correct, I tried to find a way to check that but in the end I found nothing. Anyway if by an regression the page navigates forward of the expected page, it should fail in the next test, where by a single action it won't arrive to the expected page if the current one is not the one expected prior of action.
I still have to wait for the page change event, and this checks that the action happened. At the last step we jump from page 4 to page 5(the last one) if we are already there, it will fail because no event will be triggered anymore.
If you have any suggestion on how to make this safer please guide me ;)
Comment on attachment 8509236 [details] [diff] [review]
patch v2.0

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

Just one final change to make sure we don't update this test and break that assumption in the future.

::: browser/extensions/pdfjs/test/browser_pdfjs_navigation.js
@@ +110,5 @@
> +        window = newTabBrowser.contentWindow;
> +
> +    // Runs tests after all 'load' event handlers have fired off
> +    window.addEventListener("documentload", function() {
> +      runTests(document, window, finish);

Since we know there is a potential problem with catching the page it ends on lets add a check to enforce that. Instead of immediately calling finish add a check here that after all the tests are complete we are on the last page of the pdf.
Attachment #8509236 - Flags: review?(dtownsend+bugmail) → review-
Attached patch patch v2.1Splinter Review
Done, thanks for all the help :)
Attachment #8509236 - Attachment is obsolete: true
Attachment #8510839 - Flags: review?(dtownsend+bugmail)
Comment on attachment 8510839 [details] [diff] [review]
patch v2.1

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

Looks good, thanks
Attachment #8510839 - Flags: review?(dtownsend+bugmail) → review+
Hi, can we get a try run link for this changes? Thanks!
Keywords: checkin-needed
Hi Tomcat, I'll ping you once again when it's green. 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=62d3d9eca12b
The current test passed.
Failed in a different test with a known issue https://tbpl.mozilla.org/?tree=Try&rev=62d3d9eca12b
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ae3455010063
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [pdfjs-c-testing] → [pdfjs-c-testing][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ae3455010063
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [pdfjs-c-testing][fixed-in-fx-team] → [pdfjs-c-testing]
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.