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

Update tests #759

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Update tests #759

wants to merge 2 commits into from

Conversation

kuubeu
Copy link
Collaborator

@kuubeu kuubeu commented Sep 14, 2023

New tests:

  1. using Bun in place of Node.js with chai and mocha, reducing dependencies
  2. replaced validation using the W3C validator with a local runner, v.Nu, used by W3C; decreasing validation time from ~250 ms to ~19 ms per file (tested using GitHub Actions)

Notes:

  1. looks like Bun has a bit longer setup time than Node (~2–3 s instead of ~1 s), but hopefully it gets included by default on gh-actions runners in the near future, speeding things up
  2. the Java-based validator is a bit over 31 MB and currently gets re-downloaded every time, but bun install still only takes ~600 ms, compared to ~2 s of npm install with old tests

@kuubeu kuubeu requested a review from Eiim September 14, 2023 17:13
@Eiim
Copy link
Collaborator

Eiim commented Sep 14, 2023

I've verified that this works, and quite speedily, but I do have a couple outstanding concerns:

  • bun test isn't available on Windows without WSL yet. I'm not thrilled with the idea that we'd require WSL for people to debug their SVG errors. This is perhaps an uncommon and minor enough hurdle that it's fine, but I'm on the fence about whether this is worth the bump in test speed. (Appears to be actively worked on in feat: Windows + CMake Build System oven-sh/bun#4410)
  • vnu-jar requires Java 8+ to be installed. It looks like everything will pass if you don't have Java on your path (a plausible situation) or if you have a really old version of Java (somewhat implausible)? Or maybe if there's I/O errors? I'd like to see the parsing of vnu output to be a little more robust, but I haven't really investigated the edge cases.

@kuubeu kuubeu marked this pull request as draft September 14, 2023 21:00
@kuubeu kuubeu self-assigned this Sep 14, 2023
@Eiim
Copy link
Collaborator

Eiim commented Sep 15, 2023

Some thoughts on this:

  • As for the first point, is there really any need to go to Bun? We don't get the dependency slimming if we stay with Node, which I'll agree would be nice (chai+mocha was always overkill for this project), but switching to v.Nu does at least drop fetch anyways. It seems like the speedup here is probably mostly from running v.Nu locally rather than over the internet. To the extent that Bun is faster, I imagine it's something on the order of a second for this? Probably not worth the issues it brings with native Windows, at least for now, but some data on this would be nice if it's a major motivator.
  • Can we get v.Nu to output affirmative results ("xyz.svg is valid!") and check that we have those instead? Then if any are missing we could potentially dump the full v.Nu output, if that's not too absurdly verbose. I think that should cover all the likely edge cases and allow for easier debugging.

@kuubeu
Copy link
Collaborator Author

kuubeu commented Sep 16, 2023

When I implemented this I was only thinking of running the tests via Actions, and totally forgot that someone would want to run them locally (whoops!). I chose Bun as an all-in-one tool for testing and as a learning exercise

Addressing local testing: downloading v.Nu just to check one or a couple of icons might be a bit overkill, so I'm thinking of having local tests, fetching W3C like the original test.js and compatible with Node.js, but only on changed files, and running the new tests only on GitHub Actions. What do you think of this approach?

Also I'll look into v.Nu options later today!

@Eiim
Copy link
Collaborator

Eiim commented Sep 16, 2023

I don't think it's a big deal to download v.Nu (at ~32MB, it is larger than I expected, but not unreasonably so), and I'd rather use the same thing to test locally and with Actions, since my main motivation for local running is to debug problems spotted by Actions. In particular if validator.w3.org updates sooner/later than the npm repo, it could potentially cause some headaches. I do really like the idea of running v.Nu locally to make tests a lot faster, including through Actions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants