Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using HTTPS causes the About dialog e2e license link tests to fail #1751

Open
cghague opened this issue Mar 8, 2024 · 1 comment
Open

Using HTTPS causes the About dialog e2e license link tests to fail #1751

cghague opened this issue Mar 8, 2024 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@cghague
Copy link
Contributor

cghague commented Mar 8, 2024

We noticed during the recent release of TinyPilot Pro 2.6.3 that the tests that verify the license links on the About dialog are correct fail when testing against an actual TinyPilot device.

The cause of the issue is that we're connecting to the TinyPilot device over HTTPS but without a valid certificate installed. The Playwright navigation functions ignore this, but the fetch function used to test the links fails due to the missing certificate.

We should ideally update the tests to use the Playwright navigation tools, or failing that; we should skip the tests when running in this fashion as part of our pre-release tests.

@cghague cghague added the bug Something isn't working label Mar 8, 2024
@db39 db39 self-assigned this May 29, 2024
@db39
Copy link
Contributor

db39 commented May 30, 2024

I noticed that the documentation around e2e tests isn't comprehensive. For instance, I didn't know that our e2e tests require Node 18 or greater (for fetch()), so I ended up spending unnecessary time troubleshooting that. Additionally, our process in Notion shows this command:

./dev-scripts/run-e2e-tests --base-url https://tinypilot.local

However, the script doesn't use --base-url as an argument.

Our CONTRIBUTING file mentions to turn off "Require encrypted connection (HTTPS)" to run the e2e tests. If we follow that instruction, we could probably create a workaround replacing the https base url with http so the fetch() call doesn't run into the https/certificate issue.

test("checks that all license URLs are valid and reachable", async ({
  page,
  baseURL,
}) => {
  const links = await page.locator("a.license").all();
  const paths = await Promise.all(
    links.map((link) => link.getAttribute("href"))
  );
  const http_baseURL = baseURL.replace('https://', 'http://');
  const failedUrls = [];
  await Promise.all(
    paths
      .map((path) => `${http_baseURL}${path}`)
      .map((url) =>
        fetch(url, { signal: AbortSignal.timeout(10000) })
          .then((res) => {
            if (res.status !== 200) {
              failedUrls.push(url);
            }
          })
          .catch(() => failedUrls.push(url))
      )
  );
  expect(
    failedUrls.length,
    `License link broken for URLs: ${failedUrls.join(", ")}`
  ).toBe(0);
});

I've tested the workaround, and the tests pass using it. If we use this workaround, we don't have to use navigation functions or skip the tests. Does that seem like a reasonable solution?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants