875766 - Add automated test for navigation through the document

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

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

Comment 1

[Mihaela Velimiroviciu (:mihaelav)]

I wonder if step 3 (navigation using the scroll bar) can be simulated in any way.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

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.

Comment 3

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8505349 [details] [diff] [review]
patch v1.0

Flagging other developers for review, maybe ydelendik is not available.

Comment 4

[Dave Townsend [:mossop]]

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.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 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.

> @@ +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.

Comment 6

[Dave Townsend [:mossop]]

(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?

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 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.
Also I just noticed I attached the same patch, sorry for that, here is the one with the changes you have requested.

Comment 8

[Dave Townsend [:mossop]]

(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.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

(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 10

[Dave Townsend [:mossop]]

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.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1

Done, thanks for all the help :)

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8510839 [details] [diff] [review]
patch v2.1

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

Looks good, thanks

Comment 13

[Carsten Book [:Tomcat]]

Hi, can we get a try run link for this changes? Thanks!

Comment 14

[Cosmin Malutan, [:cosmin-malutan]]

Hi Tomcat, I'll ping you once again when it's green. 
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=62d3d9eca12b

Comment 15

[Cosmin Malutan, [:cosmin-malutan]]

The current test passed.
Failed in a different test with a known issue https://tbpl.mozilla.org/?tree=Try&rev=62d3d9eca12b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae3455010063