Cleans test manifest from absent files. #6311

yurydelendik · 2015-08-04T19:51:55Z

~~Adds "disabled" property for the test with missing linked files. To enable these tests (e.g. at botio) --allowDisabled parameters shall be used with test.js.~~

Snuffleupagus · 2015-08-05T10:04:46Z

test/test_manifest.json

@@ -928,6 +929,7 @@
      "md5": "2ade57e954ae7632749cf328daeaa7a8",
      "rounds": 1,
      "link": true,
+      "disabled": true,


This appears to be the same file: http://web.archive.org/web/20100215194419/http://www.unibuc.ro/uploads_en/29535/10/Cyrillic_Alphabets-Chars.pdf (based on the old link from PR #1787).

Rob--W · 2015-08-08T11:12:25Z

What's the purpose of this patch?

Snuffleupagus · 2015-09-10T10:56:46Z

test/test_manifest.json

@@ -2267,6 +2277,7 @@
       "link": true,
       "firstPage": 9,
       "lastPage": 9,
+       "disabled": true,


If PR #6434 is accepted, then this line can be removed.

#6434 has been merged, so this line can be removed.

yurydelendik · 2015-10-30T18:37:44Z

What's the purpose of this patch?

@Rob--W , with current state of the tree the contributors efficiently cannot run tests (or tell if their change breaks stuff). This patch will clean up the manifest from links of disappeared from the internet files.

@Snuffleupagus and @brendandahl , are you okay with merging it?

timvandermeij · 2015-10-30T20:08:23Z

test/test_manifest.json

@@ -1807,7 +1813,7 @@
       "link": false,
       "type": "load",
       "password": "\u00E6\u00F8\u00E5",
-       "about": "The password (���) is UTF8 encoded."
+       "about": "The password (æøå) is UTF8 encoded."


What exactly changed in this line? I can't see anything obvious in the diff. It is some encoding thing?

timvandermeij · 2015-10-30T20:10:34Z

I see more hash changes than there are new URLs. Where do those come from exactly? Although this patch is useful, it would of course be better to replace all disabled items with either linked (from the Web Archive) or reduced test cases. @Snuffleupagus already did this for quite some tests, which was awesome, so that has my preference, but I also understand that being able to run all tests without missing PDF problems is important, so I'm fine with either way.

Snuffleupagus · 2015-11-01T09:20:43Z

I see more hash changes than there are new URLs.

I'm wondering/worrying about all of these hash changes too!
@yurydelendik Did you in every case manually confirm that the new version of a PDF file demonstrates the exact same issue as the old one, since otherwise the test might not actually test what you'd expect?

Edit: So far, I've only checked the first case of changed hashes, and there the new file version wasn't a good replacement for the old file. So it seems to me that all of the changed hashes need careful verification on a case-by-case basis.

Snuffleupagus · 2015-11-01T09:21:32Z

test/test_manifest.json

@@ -68,7 +68,7 @@
    },
    {  "id": "issue3879",
       "file": "pdfs/issue3879.pdf",
-       "md5": "1cd1f1c3271515a14e65ff2980e14663",
+       "md5": "85393be14a304d55f516918a4df889e3",


The new version of this file does not contain the same deficiency as the old one!

With PR #6585, this change shouldn't be necessary.

That PR has been merged, so this change can be removed.

Rob--W · 2015-11-01T11:10:27Z

Instead of linking to external files and hoping that they stick around, what do you think of creating a (git?) repository to host these test cases?

I have most of the PDFs locally (the oldest batch was retrieved on the 30th of May (2013)) in case you need it.

timvandermeij · 2015-11-01T13:48:54Z

@Snuffleupagus That's unfortunate, but I was also a bit afraid of that. Sometimes it takes a lot of time and effort to verify if a new file has the same issue, so perhaps we should do this in separate PRs (just like you always do) so we can test them on a case-by-case basis.

@Rob--W I believe there was a problem with redistribution. I'm all for such a Git repository, but I think that we are only allowed to link to the original PDF files if we have no permission to redistribute them. Hence I tihnk that we should replace those linked test cases with reduced test cases that we can put in our repository so we do not have that problem. Anyone who knows how to create reduced test cases (or perhaps unit tests that demonstrate the original problem of a linked PDF file) is very welcome to contribute those. I'm very happy that @Snuffleupagus is already doing such a great job with that. Maybe we should first focus on the disabled ones in this PR and try to replace them all (maybe even with a linked test case as anything is better than an unavailable test case).

Snuffleupagus · 2015-11-01T14:46:13Z

test/test_manifest.json

@@ -2470,7 +2479,7 @@
    },
    {  "id": "issue4926",
       "file": "pdfs/issue4926.pdf",
-       "md5": "ed881c8ea2f9bc4be94ecb7f2b2c149b",
+       "md5": "e0c36aaee776eea59a3b0cdbe27bd27d",


Even though this version of the PDF file seems to exhibit the same issue as the previous one, I'd still suggest updating the link to an older version instead:
http://web.archive.org/web/20140802003728/http://www.conservation.ca.gov/cgs/information/publications/cgs_notes/note_17/Documents/note_17.pdf.

My reasoning is that the old version is half the size of the new one, and it seems unnecessary to require contributors to download bigger files than strictly required.

yurydelendik · 2015-11-02T16:26:28Z

what do you think of creating a (git?) repository to host these test cases

@Rob--W As it was said above, we cannot redistribute third-party PDFs unless we will have permissions to do so. We actively request minimal test case PDFs and using linked PDFs as a last resort.

I'm thinking we are at the point when we can just discard all linked PDFs testing, however there is a tiny chance that we might regress something at fonts. I'm using this PR to:

help beginners to setup efficient testing environment
use it as a guide to determine which linked PDF must be replaced sooner
proposing intermediate solution, when we still testing linked PDFs on the servers, but contributors using smaller subset (later we can discard all linked PDF if we determine to do so)

Snuffleupagus · 2015-11-03T09:02:39Z

I'm thinking we are at the point when we can just discard all linked PDFs testing, however there is a tiny chance that we might regress something at fonts.

Unfortunately I'd be quite surprised if we could actually do this, without also losing test-coverage for a significant part of the code-base.

Based on my experience creating reduced test-cases, the real issue with removing linked tests probably isn't going to be with fonts, but rather with other parts of the code-base, e.g. the image parsing.
For fonts, you can always replace a linked test-case with a reduced one (even if it can take a fair amount of time/effort to do so). However, for test-cases related to e.g. image parsing (or certain types of PDF file corruption), it can be either very difficult or next to impossible to create reduced test-cases manually.

_Edit:_ I will, time permitting, continue to try and replace linked test-cases with reduced ones. But obviously I cannot promise neither a time-frame for this work, nor that I'll be able to successfully do so in all cases.

Snuffleupagus · 2015-11-09T12:09:06Z

test/test_manifest.json

@@ -164,11 +164,12 @@
       "link": true,
       "lastPage": 2,
       "rounds": 1,
+       "disabled": true,


With PR #6610, this change should be reverted.

That PR has been merged, so this change can be removed.

Snuffleupagus · 2015-11-16T23:39:16Z

test/test_manifest.json

@@ -1728,6 +1729,7 @@
      "md5": "89ddf9e63cac4fa2dedcfe32a62e5407",
      "rounds": 1,
      "link": true,
+      "disabled": true,


This change can be reverted, thanks to PR #6647.

Snuffleupagus · 2016-01-11T14:41:15Z

r=me, with the remaining comments resolved and the test command updated on the bots.

timvandermeij · 2016-01-11T20:02:22Z

Is there really nothing more we can do here except for disabling some test cases? I was hoping we could just replace the dead test cases with reduced ones or new links so we would not need to disable them altogether. Perhaps we can find out when some broken links were added and contact the owner of the PDF files on the original issues to see if we can get a new link/permission to add it to the test suite?

Snuffleupagus · 2016-01-11T20:38:57Z

test/test_manifest.json

@@ -2431,6 +2433,7 @@
       "link": true,
       "firstPage": 1,
       "lastPage": 2,
+       "disabled": true,


This change can be reverted once PR #6853 lands.

yurydelendik · 2016-01-13T15:44:11Z

test/test_manifest.json

@@ -1941,7 +1941,7 @@
       "link": false,
       "type": "load",
       "password": "\u00E6\u00F8\u00E5",
-       "about": "The password (���) is UTF8 encoded."
+       "about": "The password (æøå) is UTF8 encoded."


Now it's proper UTF-8 codes, not sure what encoding was used before.

timvandermeij · 2016-01-13T16:14:17Z

/botio test

pdfjsbot · 2016-01-13T16:14:18Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/77b08f895b66869/output.txt

pdfjsbot · 2016-01-13T16:14:18Z

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/95b5d5347894019/output.txt

pdfjsbot · 2016-01-13T16:35:04Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/77b08f895b66869/output.txt

Total script time: 20.77 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2016-01-13T17:14:20Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/95b5d5347894019/output.txt

Total script time: 60.00 mins

timvandermeij · 2016-01-14T15:37:25Z

/botio-linux test

pdfjsbot · 2016-01-14T15:37:26Z

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/df5e5164afaf7ee/output.txt

pdfjsbot · 2016-01-14T15:58:55Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/df5e5164afaf7ee/output.txt

Total script time: 21.49 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

Cleans test manifest from absent files.

timvandermeij · 2016-01-14T16:13:22Z

Thank you!

timvandermeij added the other label Aug 4, 2015

Snuffleupagus reviewed Aug 5, 2015
View reviewed changes

Snuffleupagus reviewed Sep 10, 2015
View reviewed changes

yurydelendik force-pushed the clean_test_manifest branch 4 times, most recently from 647fa21 to 0ec0de6 Compare October 30, 2015 18:34

timvandermeij reviewed Oct 30, 2015
View reviewed changes

Snuffleupagus reviewed Nov 1, 2015
View reviewed changes

Snuffleupagus reviewed Nov 9, 2015
View reviewed changes

yurydelendik force-pushed the clean_test_manifest branch 2 times, most recently from b6d293a to d75a3f8 Compare November 16, 2015 18:18

Snuffleupagus reviewed Nov 16, 2015
View reviewed changes

Snuffleupagus reviewed Jan 11, 2016
View reviewed changes

timvandermeij mentioned this pull request Jan 11, 2016

Make linked test cases point to the Internet Archive #6854

Closed

91 tasks

timvandermeij added test and removed other labels Jan 13, 2016

Cleans test manifest from absent files.

18f0531

yurydelendik force-pushed the clean_test_manifest branch from d75a3f8 to 18f0531 Compare January 13, 2016 15:40

yurydelendik reviewed Jan 13, 2016
View reviewed changes

timvandermeij added a commit that referenced this pull request Jan 14, 2016

Merge pull request #6311 from yurydelendik/clean_test_manifest

8318c82

Cleans test manifest from absent files.

timvandermeij merged commit 8318c82 into mozilla:master Jan 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleans test manifest from absent files. #6311

Cleans test manifest from absent files. #6311

yurydelendik commented Aug 4, 2015

Snuffleupagus Aug 5, 2015

Rob--W commented Aug 8, 2015

Snuffleupagus Sep 10, 2015

timvandermeij Oct 13, 2015

yurydelendik commented Oct 30, 2015

timvandermeij Oct 30, 2015

timvandermeij commented Oct 30, 2015

Snuffleupagus commented Nov 1, 2015

Snuffleupagus Nov 1, 2015

Snuffleupagus Nov 1, 2015

timvandermeij Nov 1, 2015

Rob--W commented Nov 1, 2015

timvandermeij commented Nov 1, 2015

Snuffleupagus Nov 1, 2015

yurydelendik commented Nov 2, 2015

Snuffleupagus commented Nov 3, 2015

Snuffleupagus Nov 9, 2015

timvandermeij Nov 12, 2015

Snuffleupagus Nov 16, 2015

Snuffleupagus commented Jan 11, 2016

timvandermeij commented Jan 11, 2016

Snuffleupagus Jan 11, 2016

yurydelendik Jan 13, 2016

timvandermeij commented Jan 13, 2016

pdfjsbot commented Jan 13, 2016

pdfjsbot commented Jan 13, 2016

pdfjsbot commented Jan 13, 2016

pdfjsbot commented Jan 13, 2016

timvandermeij commented Jan 14, 2016

pdfjsbot commented Jan 14, 2016

pdfjsbot commented Jan 14, 2016

timvandermeij commented Jan 14, 2016

Cleans test manifest from absent files. #6311

Cleans test manifest from absent files. #6311

Conversation

yurydelendik commented Aug 4, 2015

Choose a reason for hiding this comment

Rob--W commented Aug 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Oct 30, 2015

Choose a reason for hiding this comment

timvandermeij commented Oct 30, 2015

Snuffleupagus commented Nov 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W commented Nov 1, 2015

timvandermeij commented Nov 1, 2015

Choose a reason for hiding this comment

yurydelendik commented Nov 2, 2015

Snuffleupagus commented Nov 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented Jan 11, 2016

timvandermeij commented Jan 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Jan 13, 2016

pdfjsbot commented Jan 13, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Jan 13, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 13, 2016

From: Bot.io (Windows)

Success

pdfjsbot commented Jan 13, 2016

From: Bot.io (Linux)

Failed

timvandermeij commented Jan 14, 2016

pdfjsbot commented Jan 14, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Jan 14, 2016

From: Bot.io (Linux)

Success

timvandermeij commented Jan 14, 2016