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.

Sign up for GitHub

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

Jump to bottom

e2e tests fail if external license website is unreachable #1686

Open
jotaen4tinypilot opened this issue Nov 21, 2023 · 2 comments
Open

e2e tests fail if external license website is unreachable #1686

jotaen4tinypilot opened this issue Nov 21, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@jotaen4tinypilot
Copy link
Contributor

We have started to automatically check that license links in the <about-dialog> are valid, by resolving the link and checking the response status.

For license links that forward to an external website, this means we are depending on whether that external website is online. If an external license page is down, our e2e CI job fails and can block PRs.

For example, at the time of that writing, the git server of libwebsockets appears to be down, i.e. everything under https://libwebsockets.org/git/ is unresponsive. The URL path itself still seems valid, as it’s used for links on the libwebsockets homepage (which is up). Any e2e CI job currently fails after a 30s timeout.

Screenshot 2023-11-21 at 19 23 09

Solutions

Ideas:

  • (a) We just accept that this happens from time to time.
    • In this case, we could consider making the e2e test so that it fails immediately, without 30s timeout, and outputs clear logging to explain this corner-case.
  • (b) We stop following redirects when resolving the license links.
    • This, however, can produce false positives, because if e.g. a remote address is moved, we wouldn’t notice.
  • (c) We don’t make use of external redirects anymore, but manually copy and distribute the license copy as local file.
    • We would have to repeat this process whenever we bump a dependency version, however.

We currently rely on 10 external license URLs (in Pro).

Another aspect to consider is whether we strictly comply with legal requirements by just linking to external websites for the licenses, as we can’t ensure that they will be reachable at all times forever.
@jotaen4tinypilot jotaen4tinypilot added the bug Something isn't working label Nov 21, 2023
This was referenced Nov 21, 2023
Merged
Merged
@mtlynch
Copy link
Contributor

mtlynch commented Nov 21, 2023 •
edited
Loading

I think if it's down for more than a day, we switch to the Github version of the license:

https://github.com/warmcat/libwebsockets/blob/v3.2.0/LICENSE

And in general, I'm fine with sticking with checking every link unless we find that this comes up more than once per year or so.

@jotaen4tinypilot
Copy link
Contributor Author

Update: libwebsocket’s git server is up again, so our e2e tests have recovered for the time being.

jotaen4tinypilot added a commit that referenced this issue Nov 22, 2023
@jotaen4tinypilot @jotaen
Remove “Cancel” button from loading state (#1687)
59025c7 
In our style guide, we say [that dialog loading states shouldn’t have
close/cancel
buttons](https://github.com/tiny-pilot/tinypilot/blob/cbc2a379e71b1b8c2be7ac49e61e0b8a8d8c729f/app/templates/styleguide.html#L466-L468).

The `<change-hostname>` dialog violated this rule, which this PR fixes.


https://github.com/tiny-pilot/tinypilot/assets/83721279/e8e7df38-4ef9-42d0-82c8-2018b93cf0f8

Tangentially related
#1684.

~Currently blocked by
#1686 (e2e tests
failing).~

<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1687"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

Co-authored-by: Jan Heuermann <[email protected]>
jotaen4tinypilot added a commit that referenced this issue Nov 22, 2023
@jotaen4tinypilot @jotaen
Consolidate button wording in dialogs (“Close” / “Back”) (#1688)
5bdbec7 
Related #1684. Stacked
onto #1687.

As discussed in tiny-pilot/tinypilot-pro#1139,
this PR consolidates the wording for “Cancel”/“Close”/“Back”/“OK”
buttons across all our dialogs, to only use “Close” or “Back”. It also
outlines the rules in the style guide.

- In the style guide, I restructured some of the existing text, to
accommodate the new rules in a clearer way.
- The code for the demo overlays has become borderline complex. To me,
it’s still okay-ish for now, but for the future we could consider to
simplify the setup, and make it more straightforward / less interactive.
- I didn’t change the `id` attributes or CSS class names of the affected
button elements, so their wording might now diverge from the label:
e.g., we have a button labelled `Close`, with a `cancel-hostname-change`
id attribute. To me, the UI label and the internal technical name are
conceptually different enough that it wouldn’t be worth the hassle to
keep them aligned, and we already diverge in other places anyway (e.g.,
we have a button [labelled `Paste`, with a `confirm-btn` id
attribute](https://github.com/tiny-pilot/tinypilot/blob/a1aa0249bc64c8dd1ee4c75e7ea0daa982bb2d22/app/templates/custom-elements/paste-dialog.html#L30);
or a button [labelled `Apply`, with a `save-btn` id
attribute](https://github.com/tiny-pilot/tinypilot/blob/a1aa0249bc64c8dd1ee4c75e7ea0daa982bb2d22/app/templates/custom-elements/video-settings-dialog.html#L190).)
- If we’d feel that it’s worth to unify all button labels with their
`id` attribute / CSS class name, I’d suggest to do that in a separate
refactoring step, and for all buttons in one go.


https://github.com/tiny-pilot/tinypilot/assets/83721279/e426c8db-b449-4e5f-968c-1d1bbcc129eb

~Currently blocked by
#1686 (e2e tests
failing).~

<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1688"><img
src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review
on CodeApprove" /></a>

---------

Co-authored-by: Jan Heuermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mtlynch @jotaen4tinypilot