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

Plugin does not ignore commented-out expect statements or test exclusions with .only #1

Closed
huy-nguyen opened this issue Sep 16, 2017 · 7 comments · Fixed by #2
Closed

Comments

@huy-nguyen
Copy link

huy-nguyen commented Sep 16, 2017

Bug

  • babel-jest-assertions version: 0.0.1
  • node version: 8.3.0
  • npm (or yarn) version: 5.3.0

Relevant code or config

test('dummy', () => {
  // expect(1).toBe(1);
})

What you did: Run jest on the above test file.

What happened (please provide anything you think will help):jest-babel-assertions incorrectly inserts expect.asertions for the commented out expect, causing the test to fail:

expect.hasAssertions()

    Expected at least one assertion to be called but received none.

Another file that causes the same problem:

test('does not run', () => {
  expect(1).toBe(1);
})
test.only('runs', () => {
  expect(1).toBe(1)
})

In this case the error is on the does not run test.

Reproduction repository (if possible):

@huy-nguyen huy-nguyen changed the title Plugin does not ignore commented-out expect statements Plugin does not ignore commented-out expect statements or test exclusions with .only Sep 16, 2017
@mattphillips
Copy link
Owner

Hey @huy-nguyen thanks for raising this issue and nice catch!

I'm on holiday at the moment and don't have a computer with me 😢 so I won't be able to fix this right now.

How would you feel about sending a PR with the fix for the commented out expects?

The basic logic of this plugin is that it turns the test into a string and matches on all expect( patterns found to determine the count. What we need to change is the code string being used to do the match on.

On line 42 in src/index.js. You would need to put a normalised version of code variable by removing all comments in the code string. Once normalised we would use the new variable to determine the count, hasAssertions and expect.assertions already exists.

const normalisedCode = code.replace(/\/\/(.*)/g, '').replace(/\/\*([\s\S]*)\*\//g, '');

If you could add test cases for this with the following formats that would be great.

test('returns 2 when given 1 and 1', () => {
  /*
  expect(1).toBe(1);
  */
  const a = 1; /* expect(a).toBe(1) */
  const b = 1; // expect(b).toBe(1)
  expect(add(a, b)).toBe(2);
  // expect (add(1, 2)).toBe(3);
})

If you don't have time to send a PR no worries dude just let me know and I'll try and find someone who can help in my absence 😄

@mattphillips
Copy link
Owner

As for the second issue with .only tests, this appears to be an issue in jest see. Until this is fixed the hasAssertions will break in this plugin, perhaps we should remove it until then?

@hiddentao
Copy link
Contributor

PR in #2

@mattphillips
Copy link
Owner

Thanks @huy-nguyen for the issue and @hiddentao for the PR. Now got to figure out how to release without a computer 😂

Once I've published the fix I'll let you know.

@mattphillips
Copy link
Owner

Re-opening for the issue with focussed tests and Jests hasAssertions

@mattphillips
Copy link
Owner

@huy-nguyen Jest hasAssertions has been fixed in the above PR, just waiting on a release of Jest.

@mattphillips
Copy link
Owner

I've just published v0.0.2 with the fix for commented out expects and the issue in jest should be released within a week 🎉

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 a pull request may close this issue.

3 participants