schema@ draft-7, no longer exporting validator, using jest for tests

[antialias]

separating all these things is a big difficult so grouping them in the same branch for this PR.

Also, ajv wouldn't work until I said that the schema was draft-7.

Why this code is all in one PR:

1. In removing the validator from the public export, we still need use something when testing the schema, so ajv has been added as a devDependency, and its usage is consistent with the changes made to readme.md
2. ajv validates sync by default (which is preferred for running unit tests).
3. the current tests were written to run asynchronously, so they all had to be rewritten. (ajv can also validate async, but only as a promise so these tests would have to be rewritten anyway in order to use ajv.)
4. jest:
   • has a much more concise syntax for making expectations (making the rewrite much quicker and more concise). (It also supports inline snapshots, so we can have the tests write themselves if we want)
   • can run the 198 tests in parallel test runners so it's much faster
   • is how modern js codebases are tested

[evanplaice]

I don't understand the purpose of this PR

Why switch all the tests to Jest? The validator needs to stay until 1.0.

[antialias]

I don't understand the purpose of this PR

glad you asked :)

Why switch all the tests to Jest?

1. In removing the validator from the public export, we still need use something when testing the schema, so ajv has been added as a devDependency, and its usage is consistent with the changes made to readme.md
2. ajv validates sync by default (which is preferred for running unit tests).
3. the current tests were written to run asynchronously, so they all had to be rewritten. (ajv can also validate async, but only as a promise so these tests would have to be rewritten anyway in order to use ajv.)
4. jest:
   • has a much more concise syntax for making expectations (making the rewrite much quicker and more concise). (It also supports inline snapshots, so we can have the tests write themselves if we want)
   • can run the 198 tests in parallel test runners so it's much faster
   • is how modern js codebases are tested

The validator needs to stay until 1.0.

Is that inclusive? (i.e. do we want the validator to appear in 1.0.0?)

[evanplaice]

I get 1, 2, 3

can run the 198 tests in parallel test runners so it's much faster

The entire test suite runs in <1 second using Tape. Running unit tests in parallel is only really necessary if you have to front-load a ton of code for each test (ex a framework app testing).

is how modern js codebases are tested

Popular != modern. Jest is popular for FrontEnd testing b/c it comes with JSDOM and a ton of other features. To support that Jest also requires a massive tree of dependencies. Read. more packages that need to be kept up-to-date and more packages where potential security vulnerabilities could be found in the future.

The goal is to reduce not increase the long-term maintenance burden here

Is that inclusive? (i.e. do we want the validator to appear in 1.0.0?)

The validator appears in everything up to 1.0 pre-releases. If the validation/test updates were isolated into a separate PR that work could be backlogged until the work on pre-releases starts. This PR has a ton of cross-cutting concerns, which will lead to merge conflicts leading up to 1.0.

---

Don't get me wrong. The work switching validation over and converting the tests to sync is 👍

[antialias]

This PR has a ton of cross-cutting concerns, which will lead to merge conflicts leading up to 1.0.

The only impact here is in the test code, so unless we wind up rewriting them again, this shouldn't raise a concern about future merge conflicts.

[antialias]

The validator appears in everything up to 1.0 pre-releases. If the validation/test updates were isolated into a separate PR that work could be backlogged until the work on pre-releases starts.

I'm still unclear if the plan is to include the validator in the 1.0.0 release. "up to" and "until" leave inclusion in the subject ambiguous. There's a fine course of action for either scenario :)

[evanplaice]

Sorry. Cross-cutting concerns = README.md, there's other cleanup work that still needs to take place there. 1.0 will not include validator.js or export a validator; a resume-cli should be switched over before resume-schema is bumped > 1.0 there.

Except for not switching to Jest, the only thing other thing I'd change is to remove the util folder and inline the validator creation; unless you think that'll be something that changes in the future.

[antialias]

Popular != modern.

jest is modern compared to tape.

Jest is popular for FrontEnd testing b/c it comes with JSDOM and a ton of other features.

jest is popular for javascript, for example, @babel/core runs is node-only and uses jest for all its testing.

it comes with JSDOM

I have testEnvironment: 'node' set so no jsdom stuff is loaded when the tests are run.

Read. more packages that need to be kept up-to-date and more packages where potential security vulnerabilities could be found in the future.

Jest is a single package in this project and Facebook does a great job keeping it current; last update was 4 days ago (tape, was last updated two months ago by substack). Who would you bet is going to be more responsive to vulnerabilities?

The goal is to reduce not increase the long-term maintenance burden here

That's what I was going for by updating the tests to use the de-facto api pattern of expect().... Mocha and jasmine use the same api, so if you really hate jest we can easily switch over to one of those harnesses. Making that switch becomes harder if we stick with tape's api.

To support that Jest also requires a massive tree of dependencies.

Since jest is declared as a devDependency, it never gets installed by projects that depend on resume-schema, so this isn't something to be concerned about.

[antialias]

Cross-cutting concerns = README.md, there's other cleanup work that still needs to take place there.

ok, let's get that taken care of as well. Do you have a work in progress that conflicts with it? If yes, I can rebase on top of it. If no, best to get this merged in sooner so we can avoid merge conflicts.

1.0 will not include validator.js or export a validator;

This PR targets the v1.0.0 branch and removes the validator as planned. Why would we want to isolate changes that remove the validator?

resume-cli should be switched over before resume-schema is bumped > 1.0 there.

didn't we do that yesterday?

remove the util folder and inline the validator creation; unless you think that'll be something that changes in the future.

Sure, but can you explain why? Maybe rename it test-utils? I wanted to keep that code a bit separate from the main exports of this package since it is only supposed to be used for the tests.

[evanplaice]

ok, let's get that taken care of as well. Do you have a work in progress that conflicts with it? If yes, I can rebase on top of it. If no, best to get this merged in sooner so we can avoid merge conflicts.

Not yet, I can have PRs posted for review today.

This PR targets the v1.0.0 branch and removes the validator as planned. Why would we want to isolate changes that remove the validator?

👍

didn't we do that yesterday?

On the master branch, yes. The change still needs to be published to NPM. I'd like to -- at least attempt -- to resolve some of the outdated deps and security vulns before publishing another NPM release.

Sure, but can you explain why? Maybe rename it test-utils? I wanted to keep that code a bit separate from the main exports of this package since it is only supposed to be used for the tests.

If that validation code will never change there's no need to abstract it out into a separate module. Why? Reducing # of files as a subtle UX improvement.

If you think the validator creation will need to change in the future, then it makes sense to have it abstracted out into a separate module.

[antialias]

resume-cli should be switched over before resume-schema is bumped > 1.0 there.

didn't we do that yesterday?

On the master branch, yes. The change still needs to be published to NPM. I'd like to -- at least attempt -- to resolve some of the outdated deps and security vulns before publishing another NPM release.

The order that things are done in this repo doesn't affect how resume-cli functions, so there's no reason to wait for resume-cli before improving the code in this repo, and vice versa.

[antialias]

Sure, but can you explain why? Maybe rename it test-utils? I wanted to keep that code a bit separate from the main exports of this package since it is only supposed to be used for the tests.

If that validation code will never change there's no need to abstract it out into a separate module. Why? Reducing # of files as a subtle UX improvement.

If you think the validator creation will need to change in the future, then it makes sense to have it abstracted out into a separate module.

utils/validate.js is shared between the 14 *.spec files in __test__. It could be inlined into them, but it's nicer to share the validator set-up between the spec files.

[antialias]

This PR has a ton of cross-cutting concerns, which will lead to merge conflicts leading up to 1.0.
Cross-cutting concerns = README.md

ack. that the change under "contribute" is out of scope; happy to remove it if that helps avoid merge conflicts.

[evanplaice]

If you see a lot of noise in your notifications. I'm backfilling tags, releases, and the CHANGELOG.

When I'm done, it's free game to change whatever in README.md.

[evanplaice]

Took longer than expected (messy history) but you should be unblocked on making changes to README.md now.

[antialias]

Cool! Unsure where we stand at this point in the conversation about this PR. Can we collect any outstanding concerns so they can be addressed?

[evanplaice]

Use Tape not Jest

[antialias]

Okay, looks like moving to jest is the only remaining concern we have to address in this PR. Firstly, can you ack my last comment on the subject before we proceed; I'd rather keep my points in single comments.

Some data on the concern of vulnerabilities, I just compared runs of audit on local clones of master of jest and tape and got this.

jest:

➜  jest git:(master) yarn audit
...
5 vulnerabilities found - Packages audited: 919720
Severity: 3 Low | 1 Moderate | 1 High

severity	type
low	Prototype Pollution
low	Prototype Pollution
moderate	Prototype Pollution
high	Cross-Site Scripting
low	Prototype Pollution

tape:

➜  tape git:(master) ✗ yarn audit
...
12 vulnerabilities found - Packages audited: 1956
Severity: 4 Low | 6 Moderate | 2 High

severity	type
low	Regular Expression Denial of Service
low	Regular Expression Denial of Service
moderate	Denial of Service
high	Code Injection
low	Prototype Pollution
moderate	Prototype Pollution
moderate	Prototype Pollution
moderate	Prototype Pollution
moderate	Prototype Pollution
moderate	Memory Exposure
low	Denial of Service
high	Insufficient Entropy

tape also requires that we use tap-spec. audit shows no vulns, but it was last published over two years ago.

tape was a reasonable choice when it was added to this project to run the first tests over five years ago in 2014. Javascript testing has evolved considerably since then, and it would be a mistake to ignore that. Just one example is snapshot testing, and it would be immediately useful once we begin evolving the spec or adding migration functions. Jest also gives nice coverage reports with almost no extra configuration.

[evanplaice]

I ack the previous comment. My take still stands.

I have used Jest plenty for FrontEnd dev, snapshot testing is great for comparing rendered DOM trees and such.

For super basic unit testing (ie output = result) Tape is a better fit. Testing the schema or CLI really doesn't need to be more complex than that.

In terms of potential breaks (ie security or out-of-date deps), I was referring to surface area. Ie 919k vs 1.9k.

Tape outputs TAP test results, it's a standardized test reporting standard. Tap-spec is a TAP formatter. It just pretty prints the results. Feel free to remove it if you want.

If you want 'modern' use a lib that is Node + ESM compatible. Of which Jest isn't. But that isn't really necessary here unless the entire Node ecosystem deprecates CommonJS in the future.

I know Tape is old, I've been using it for OSS lib testing for years. It's also very stable, and lightweight for general-use unit testing. Which is exactly what we need here.

Keep in mind, nobody is paid to work on this project. Reducing the maintenance burden to a minimum is good for the long-term health and sustainability of the project.

That's my overarching end goal. Address all of the feedback that has been left to rot due to neglect. And reduce the effort to keep this project alive so it doesn't require a lot of effort for I -- and future maintainers -- to keep it alive.

[antialias]

In terms of potential breaks (ie security or out-of-date deps), I was referring to surface area. Ie 919k vs 1.9k.

How does my data from audit not trump this? Jest's vulnerabilities are more actively triaged and the software itself has a better maintenance record. It's also trusted by more projects in the javascript community than any other. ... and it's only devDependency, so this concern over vulns really doesn't matter at all since it doesn't get installed in dependent projects.

I have used Jest plenty for FrontEnd dev, snapshot testing is great for comparing rendered DOM trees and such.

That something works great for FE jobs doesn't mean that it's only for FE work. I've already cited babel as an example of significant non-FE project that uses jest for testing. Other examples of completely server-side and high-profile projects that use jest to test are: webpack, jest itself, apollo-server, and ncc. Of note, when I was browsing the most actively maintained projects for node, I didn't find any projects that still use tape.

If you want 'modern' use a lib that is Node + ESM compatible. Of which Jest isn't.

Is tape? Jest respects babel.config.js which in practice is where the state of the art is. I have not come across any .mjs files in the real world since they were introduced several years ago.

I'm not after modern for modern's sake – if we're looking to get this project back on track and provide a nice experience for contributors, maybe we want to use something that most devs are likely to have used recently, and that also provides a first class experience for them?

For super basic unit testing (ie output = result) Tape is a better fit. Testing the schema or CLI really doesn't need to be more complex than that.

Jest is as simple as we need it to be. Config defaults are sane and the expectation api is concise. And it can scale with the project as we need it to.

Also, I'd bet that the next time these test are re-written, we're going to want to have them generated (as was suggested in another thread.)

Keep in mind, nobody is paid to work on this project.

I'd bet that most folks that find their way here from the professional world are probably more familiar with jest than they are with tape.

Reducing the maintenance burden to a minimum is good for the long-term health and sustainability of the project.

Jest is not a maintenance burden.

My crystal ball says that we will eventually move to something more in line with the api that all the other testing frameworks have converged upon.

---

I'm done with this branch, but if you want to submit a PR against it that moves everything back to tape, that will be fine.

[evanplaice]

I have not come across any .mjs files in the real world since they were introduced several years ago.

I have an entire org dedicated to ESM + Node. I'm also a member of the node/modules team. You don't see it in the wild b/c the new implementation was just unflagged in January and 'experimental' warnings were removed a week ago.

I've spent the past 2 years arguing about the 'future of JS'. TBH, I'm enjoying working on something that doesn't require arguing about the future of JS for once.

---

I understand you're not a fan. Don't worry about it. I'll take care of the rest.

[evanplaice]

In Tape tests the t.done() is only necessary for async testing. Each test should only require a t.equal() statement. If you want a hand let me know. My backlog for cleanup work in this repo is mostly finished.

[antialias]

bumped jest to v26 in anticipation of jest28, which "will remove ... jest-environment-jsdom from the default distribution of Jest".