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

Updates jest-validate to v25.1.0 #122

Closed
wants to merge 1 commit into from
Closed

Updates jest-validate to v25.1.0 #122

wants to merge 1 commit into from

Conversation

opiation
Copy link

@opiation opiation commented Mar 5, 2020

Hey there,

I'm not sure if this is a helpful PR or not. Feel free to disregard and close this it if this is not in your roadmap or interests.

The only related breaking change mentioned in the jest changelog comes in v25.1.0 and is a switch to using ESM. I'm uncertain if this is acceptable to this project as there is no mention of officially supported or unsupported versions of node or jest in the README.md.

EDIT: I should probably have assumed the versions 8, 10 and 12 listed in .travis.yml are the official supported node versions and 22 to <25 for jest respectively...

@palmerj3
Copy link
Collaborator

palmerj3 commented Mar 6, 2020

Hey thanks for the PR!

I will try to look into this more this weekend. But I wanted to respond first so you know I've seen this :)

@nickrobson
Copy link

Hey there! Have you been able to give this a look, @palmerj3 ? Obviously if you haven't then no worries, I'm just curious and interested in updating to use this change when it's available. Thanks in advance! :)

@palmerj3
Copy link
Collaborator

Hello again - so sorry for the delays.

I am finally getting to looking at this PR.

My biggest concern with going this route is that we'd have to actually include a bundler and a build step pre-publish in order to support this. It's not as simple as just bumping the dependency.

We need to continue to support the same node versions as Jest so we need to be able to support as low as 8.x and it's only 13.x that natively supports ESM is that right?

@opiation
Copy link
Author

Hey @palmerj3, thanks for getting back to me. I hear your concerns but I don't think any support is lost here, unless I'm missing something.

I have not encountered any errors using this PR with node 8 (nor 10, 12, 13, or 14). I tested the following:

  1. Create a project with jest v25.5.4, this PR for jest-junit (using a local package) and nodejs v8.17.0
  2. Added a single test
function answer() { return 42 }

describe(answer, () => {
  it('returns 42', () => {
    expect(answer()).toEqual(42)
  })
})
  1. Included jest-junit in the reporters config property of jest.confnig.js (as in the docs)
  2. Ran tests successfully yarn run jest
  3. Verified the generated junit.xml
<?xml version="1.0" encoding="UTF-8"?>
<testsuites name="jest tests" tests="1" failures="0" time="0.449">
  <testsuite name="answer" errors="0" failures="0" skipped="0" timestamp="2020-05-17T02:00:54" time="0.409" tests="1">
    <testcase classname="answer returns 42" name="answer returns 42" time="0.003">
    </testcase>
  </testsuite>
</testsuites>

Is there any other supporting evidence required to make this PR acceptable? Also, from the .travis.yml config, node 8, 10 and 12 appear to be tested with the integration-tests in the repo (which have passed for this PR). Is there any reason to believe those would give false positives if the PR could not be run used by a project with node 8/10/12?

@SimenB
Copy link
Member

SimenB commented May 17, 2020

Jest doesn't ship ESM code, the changelog entry refers to the authoring if the module - it's still transpiled to CJS

@palmerj3
Copy link
Collaborator

Closing this PR since I just removed jest-validate all together - really isn't super necessary and this makes it much easier to manage.

@palmerj3 palmerj3 closed this Jun 22, 2020
@opiation opiation deleted the chore/update-jest-validate branch June 24, 2020 00:25
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.

4 participants