resume-schema
resume-schema copied to clipboard
jsonresume
schema@ draft-7, no longer exporting validator, using jest for tests
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:
- In removing the validator from the public export, we still need use something when testing the schema, so
ajvhas been added as a devDependency, and its usage is consistent with the changes made to readme.md -
ajvvalidates sync by default (which is preferred for running unit tests). - the current tests were written to run asynchronously, so they all had to be rewritten. (
ajvcan also validate async, but only as a promise so these tests would have to be rewritten anyway in order to useajv.) -
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
I don't understand the purpose of this PR
Why switch all the tests to Jest? The validator needs to stay until 1.0.
I don't understand the purpose of this PR
glad you asked :)
Why switch all the tests to Jest?
- In removing the validator from the public export, we still need use something when testing the schema, so
ajvhas been added as a devDependency, and its usage is consistent with the changes made to readme.md -
ajvvalidates sync by default (which is preferred for running unit tests). - the current tests were written to run asynchronously, so they all had to be rewritten. (
ajvcan also validate async, but only as a promise so these tests would have to be rewritten anyway in order to useajv.) -
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?)
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 :+1:
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.
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 :)
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.
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.
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.
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?
:+1:
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.
resume-cli should be switched over before resume-schema is bumped > 1.0 there.
didn't we do that yesterday?
On the
masterbranch, 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.
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.
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.
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.
Took longer than expected (messy history) but you should be unblocked on making changes to README.md now.
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?
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.
bumped jest to v26 in anticipation of jest28, which "will remove ... jest-environment-jsdom from the default distribution of Jest".