Add instrumented tests and js tests#627
Conversation
ggtlvkma356
force-pushed
the
Espresso-Web
branch
3 times, most recently
from
6dd6ed5 to
7a61ba4
Compare
inthewaves
left a comment
inthewaves
left a comment
There was a problem hiding this comment.
Coverage looks good, just some gaps we could add
| @Test | ||
| fun openSecondDocument_resetsStateAndLoadsNewDocument() { | ||
| PdfViewerLauncher.launchWithTestAsset("test-multipage.pdf").use { scenario -> | ||
| PdfViewerTestUtils.waitForDocumentFullyLoaded(scenario) | ||
| PdfViewerTestUtils.waitForCanvasRendered(scenario) | ||
|
|
||
| scenario.onActivity { | ||
| it.onJumpToPageInDocument(3) | ||
| } | ||
| scenario.onActivity { | ||
| assertEquals(3, it.currentPage) | ||
| assertEquals(4, it.totalPages) | ||
| } | ||
|
|
||
| Intents.init() | ||
| try { | ||
| val secondUri = PdfViewerLauncher.testAssetUri("test-simple.pdf") | ||
| intending(hasAction(Intent.ACTION_OPEN_DOCUMENT)) | ||
| .respondWith( | ||
| Instrumentation.ActivityResult( | ||
| Activity.RESULT_OK, | ||
| Intent().apply { data = secondUri } | ||
| ) | ||
| ) | ||
|
|
||
| robot.click(PdfViewerRobot.AppMenuItem.Open) | ||
| } finally { | ||
| Intents.release() | ||
| } | ||
|
|
||
| PdfViewerTestUtils.waitForDocumentChanged(scenario, expectedPages = 1) | ||
| PdfViewerTestUtils.waitForCanvasRendered(scenario) | ||
|
|
||
| scenario.onActivity { | ||
| assertEquals("Page should reset to 1", 1, it.currentPage) | ||
| assertEquals("New document should have 1 page", 1, it.totalPages) | ||
| } | ||
|
|
||
| PdfViewerTestUtils.assertTextLayerContent(scenario, "Test Text") | ||
| } | ||
| } |
There was a problem hiding this comment.
This test format is great: loading PDF A, doing some stuff, then load PDF B and verify all state is reset
Currently, this only tests that the page is reset, but there's other state could assert for as well (e.g., zoom state, rotation)
Also we could add some other test cases too, like
- loading test-multipage.pdf which has an outline, checking that outline is visible in menu, then load some other PDF that doesn't have an outline, and ensure outline option is no longer shown in menu (and vice versa)
- opening non-encrypted PDF first, then open encrypted PDF and ensure it still asks for password
There was a problem hiding this comment.
I have added these tests. However, for rotation, current it is not reset after loading a second pdf, so the assertion is commented out. I think this is a bug. The rest works correctly.
There was a problem hiding this comment.
We can open an issue for this then
| @Override | ||
| public ParcelFileDescriptor openFile(Uri uri, @NonNull String mode) throws FileNotFoundException { | ||
| String filename = uri.getLastPathSegment(); | ||
| if (filename == null) { | ||
| throw new FileNotFoundException("URI must include a filename"); | ||
| } | ||
|
|
||
| File file = new File(getContext().getCacheDir(), filename); | ||
| if (!file.exists()) { | ||
| try { | ||
| InputStream is = getContext().getAssets().open(filename); | ||
| FileOutputStream os = new FileOutputStream(file); | ||
| byte[] buffer = new byte[8192]; | ||
| int read; | ||
| while ((read = is.read(buffer)) != -1) { | ||
| os.write(buffer, 0, read); | ||
| } | ||
| os.close(); | ||
| is.close(); | ||
| } catch (IOException e) { | ||
| throw new FileNotFoundException( | ||
| "Failed to copy asset '" + filename + "': " + e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| return ParcelFileDescriptor.open(file, ParcelFileDescriptor.MODE_READ_ONLY); | ||
| } |
There was a problem hiding this comment.
It might be worth adding a test for content provider open failure in middle of reading file (e.g. opening document from cloud provider or USB gets unplugged). Could do this like a query parameter on the Uri that would make it throw an exception partway
There was a problem hiding this comment.
I added the test, but for the current implementation it doesn't do much, because the UI didn't show error if I'm not mistaken. Adding some feedback will be a separate PR.
There was a problem hiding this comment.
We can open an issue for this error reporting then
inthewaves
left a comment
inthewaves
left a comment
There was a problem hiding this comment.
Also, I'm sure this seems a bit trivial (I think so as well), but it could be good for reproducibility to include the .tex files used to generate the PDFs somewhere (or put them all in one markdown in separate code blocks)
ggtlvkma356
force-pushed
the
Espresso-Web
branch
from
bc6b1a6 to
548c8ee
Compare
I have added a Markdown file in the |
ggtlvkma356
force-pushed
the
Espresso-Web
branch
from
548c8ee to
9472385
Compare
2 participants
Adds instrumented tests that covers most of the user interactions.
The JS tests only cover outline, because
pdf.jsalso has their own tests, which covers most cases already.The changes to production code is minimal: