Add test cases for redirected responses #19074
Conversation
| pending("Cannot simulate cross-origin requests in Node.js"); | ||
| } | ||
| const params = buildGetDocumentParams(filename, options); | ||
| const url = new URL(params.url); | ||
| if (url.hostname === "localhost") { | ||
| url.hostname = "127.0.0.1"; | ||
| } else if (params.url.hostname === "127.0.0.1") { |
There was a problem hiding this comment.
Note: this params.url.hostname was always incorrect, it should have been url.hostname.
Rob--W
force-pushed
the
issue-12744-test
branch
from
2e06cf6
to
bb89389
Compare
|
Note: rebase this when #19028 is merged. The new test is expected to fail right now because the fix has not been applied yet. |
Rob--W
force-pushed
the
issue-12744-test
branch
2 times, most recently
from
914df5d
to
1e53500
Compare
|
@Snuffleupagus I have addressed the nits, and also updated the PR to run the tests for all IPDFStream implementations where this is relevant, instead of just PDFNetworkStream - i.e. I repeated the test for PDFFetchStream. |
Some tests rely on the presence of a server that serves PDF files. When tests are run from a web browser, the test files and PDF files are served by the same server (WebServer), but in Node.js that server is not around. Currently, the tests that depend on it start a minimal Node.js server that re-implements part of the functionality from WebServer. To avoid code duplication when tests depend on more complex behaviors, this patch replaces createTemporaryNodeServer with the existing WebServer, wrapped in a new test utility that has the same interface in Node.js and non-Node.js environments (=TestPdfsServer). This patch has been tested by running the refactored tests in the following three configurations: 1. From the browser: - http://localhost:8888/test/unit/unit_test.html?spec=api - http://localhost:8888/test/unit/unit_test.html?spec=fetch_stream 2. Run specific tests directly with jasmine without legacy bundling: `JASMINE_CONFIG_PATH=test/unit/clitests.json ./node_modules/.bin/jasmine --filter='^api|^fetch_stream'` 3. `gulp unittestcli`
Rob--W
force-pushed
the
issue-12744-test
branch
from
1e53500
to
88f309c
Compare
|
@Snuffleupagus I've updated the test by moving it to a separate file. After doing that, I ran into a new issue: while the network_spec.js test ran in the browser only, the fetch_stream_spec.js test was also run in Node.js. I looked into re-using the existing "temporary node server" helper, but then I would have to re-implement the redirect functionality of webserver.mjs there. So in the end I decided to prepend another commit that replaces |
|
Locally (on Windows) the new tests unfortunately seem to fail when running /botio unittest |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Rob--W
force-pushed
the
issue-12744-test
branch
from
88f309c
to
26d71f3
Compare
Sorry about that, I accidentally lost some lines while rebasing. This is the diff compared to before: diff --git a/test/unit/network_spec.js b/test/unit/network_spec.js
index 4c2624a8b..367e52560 100644
--- a/test/unit/network_spec.js
+++ b/test/unit/network_spec.js
@@ -16,6 +16,7 @@
import { AbortException } from "../../src/shared/util.js";
import { PDFNetworkStream } from "../../src/display/network.js";
import { testCrossOriginRedirects } from "./common_pdfstream_tests.js";
+import { TestPdfsServer } from "./test_utils.js";
describe("network", function () {
const pdf1 = new URL("../pdfs/tracemonkey.pdf", window.location).href;
@@ -118,6 +119,14 @@ describe("network", function () {
});
describe("Redirects", function () {
+ beforeAll(async function () {
+ await TestPdfsServer.ensureStarted();
+ });
+
+ afterAll(async function () {
+ await TestPdfsServer.ensureStopped();
+ });
+
it("redirects allowed if all responses are same-origin", async function () {
await testCrossOriginRedirects({
PDFStreamClass: PDFNetworkStream,
|
Rob--W
force-pushed
the
issue-12744-test
branch
from
26d71f3
to
76d9753
Compare
|
Oddly, the test passes in Firefox but failed in Chrome, with the following error:
I updated the patch to add a cache buster, to see if that fixes the test. /botio unittest |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/efefda0bf24c87e/output.txt Total script time: 2.48 mins
|
|
??? The test output is inconsistent. Within the same file in the test on Linux, I see the following (one for network, one for fetch_stream, unclear which is which): http://54.241.84.105:8877/efefda0bf24c87e/output.txt
|
Regression tests for issue mozilla#12744 and PR mozilla#19028
Rob--W
force-pushed
the
issue-12744-test
branch
from
76d9753
to
b23e9a4
Compare
|
Another attempt - adding Vary:Origin at the server side instead of cache buster at the client side. |
|
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Rob--W received. Current queue size: 0 Live output at: http://54.193.163.58:8877/cf821e7cebb656b/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_unittest from @Rob--W received. Current queue size: 0 Live output at: http://54.241.84.105:8877/203b9ff7350ccec/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/203b9ff7350ccec/output.txt Total script time: 2.53 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/cf821e7cebb656b/output.txt Total script time: 6.44 mins
|
|
I suspect that Chrome serves the range response directly from its own cache (based on the response from the full request), without reaching the server, and thus never triggering the redirect, and therefore not triggering the expected failure. I'll look into a way to avoid that, later. |
These are regression tests for issue #12744 and PR #19028.