Skip to content

Conversation

jcking
Copy link
Collaborator

@jcking jcking commented Jan 19, 2022

Migrate from jest to jasmine, resulting in a significant drop in dependencies including dropping yarn which was unnecessary. AFAIK jest is built on jasmine, so with this change we are just cutting out the middle man.

@jcking jcking marked this pull request as ready for review January 19, 2022 16:36
@jcking
Copy link
Collaborator Author

jcking commented Jan 19, 2022

@parrt

@parrt
Copy link
Member

parrt commented Jan 20, 2022

I'm not really qualified to review this change. @ericvergnaud how does this look?

@ericvergnaud
Copy link
Contributor

  1. the dependencies were there to force package versions in order to avoid dependabot security warnings, so not sure this is a simplification, since we'll probably have to bring them (or others) back ?

  2. re yarn, I don't mind removing it, have you tried using jest from npm ? it should work now (I think it didn't when I created these tests)

  3. re jasmine, well isn't that a developer preference ? what's the value of jasmine over jest ? can you provide a reference re "AFAIK jest is built on jasmine" I couldn't locate one ?

@jcking
Copy link
Collaborator Author

jcking commented Jan 20, 2022

  1. the dependencies were there to force package versions in order to avoid dependabot security warnings, so not sure this is a simplification, since we'll probably have to bring them (or others) back ?

The versions should be forced by the package-lock.json, which ensures its reproducible. It also looks like jasmine has far fewer transitive dependencies then jest, the package-lock.json is much smaller.

  1. re yarn, I don't mind removing it, have you tried using jest from npm ? it should work now (I think it didn't when I created these tests)

It looks like either jest or jasmine will work the same in this regard.

  1. re jasmine, well isn't that a developer preference ? what's the value of jasmine over jest ? can you provide a reference re "AFAIK jest is built on jasmine" I couldn't locate one ?

It looks like I misinterpreted the original documentation I saw, they said jest is based on jasmine. However it looks like they meant inspired by. Both jest and jasmine appear to have the same top-level APIs for tests, but I did not see clear evidence jest actually uses jasmine internally. More they are just mostly compatible.

I think the main argument for jasmine over jest is jasmine has very fewer transitive dependencies. Both APIs are pretty much compatible.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Jan 20, 2022

The versions should be forced by the package-lock.json, which ensures its reproducible.

Not sure I follow your reasoning ... package-lock.json is generated from package.json, so if one needs a security update on a dependency, they still need to update package.json (which is how the current one got a bit bloated)

@ericvergnaud
Copy link
Contributor

I do tons of react stuff, for which jest is better suited, so they say...
Switching from jest to jasmine could be better for this product, but not so much for me...
I'm willing to accept this PR, but if jasmine gets in my way I'l switch back to jest.
Are you ok with that ?

@jcking
Copy link
Collaborator Author

jcking commented Jan 20, 2022

I do tons of react stuff, for which jest is better suited, so they say... Switching from jest to jasmine could be better for this product, but not so much for me... I'm willing to accept this PR, but if jasmine gets in my way I'l switch back to jest. Are you ok with that ?

Fine with me. Switching back to jest should be relatively trivial.

@jcking
Copy link
Collaborator Author

jcking commented Jan 20, 2022

The versions should be forced by the package-lock.json, which ensures its reproducible.

Not sure I follow your reasoning ... package-lock.json is generated from package.json, so if one needs a security update on a dependency, they still need to update package.json (which is how the current one got a bit bloated)

I think npm audit --fix can be used for that without updating package.json. https://docs.npmjs.com/cli/v8/commands/npm-audit

@parrt parrt added this to the 4.10 milestone Jan 21, 2022
@parrt
Copy link
Member

parrt commented Jan 21, 2022

Sounds like we're safe to merge?

@ericvergnaud ericvergnaud merged commit 2f9102a into antlr:master Jan 21, 2022
@ericvergnaud
Copy link
Contributor

Done

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

Successfully merging this pull request may close these issues.

3 participants