feat(expect-expect): support using wildcards in additionalTestBlockFunctions
#994
Conversation
G-Rath
changed the title
additionalTestBlockFunctions
There was a problem hiding this comment.
Overall looks good (minus the failing tests), but I think it would be good to have a changelog entry for each rule which means either doing a PR for each rule or having one PR with only two commits (that follow the conventional commit format), one for expect-expect & another for no-standalone-expect.
Let me know if you'd like me to help, or if you'd prefer I take care of that side of things.
(I'd say it's probably best to do two different PRs, as it looks like the change for no-standalone-expect has broken other tests, which could be because the regex is not suitable).
| @@ -141,5 +146,38 @@ describe('NumberToLongString', () => { | |||
| expect(output).toBe(theory.expected); | |||
| }, | |||
| ); | |||
|
|
|||
| it('should be correctly translated to string', () => | |||
| expectToBeCorrectString(NumberToLongString(100), 'One hundred')); | |||
There was a problem hiding this comment.
nit: would prefer to have a body on this function, to make it simpler
| @@ -125,6 +126,10 @@ The following is _correct_ when using the above configuration: | |||
| ```js | |||
| import theoretically from 'jest-theories'; | |||
|
|
|||
| function expectToBeCorrectString(result, expectedString) { | |||
| return expect(result).toBe(expectedString); | |||
There was a problem hiding this comment.
nit: let's remove the return, since vanilla expect throws instead of returning and so it should be less chance of confusing developers
| return expect(result).toBe(expectedString); | |
| expect(result).toBe(expectedString); |
| `, | ||
| options: [ | ||
| { | ||
| assertFunctionNames: ['assert*'], |
There was a problem hiding this comment.
We should be able to have a test case without this option being provided, to keep things simple (having both test cases is good too).
| `, | ||
| options: [ | ||
| { | ||
| assertFunctionNames: ['assert*'], |
There was a problem hiding this comment.
We should be able to have a test case without this option being provided, to keep things simple (having both test cases is good too).
Additionally, this test is currently failing because assertFunctionNames is meant to be all of the assertion functions to allow - i.e. it's default is ['expect'], meaning if you pass a custom value you need to explicitly include 'expect'.
|
@NickBolles are you still working on this? |
|
@G-Rath totally forgot about it. I'll come back to it sometime in the next week |
|
@NickBolles are you willing to work on this, or are you happy if I pick it up? |
@G-Rath You're welcome to pick it up, otherwise I'd still like to get to it.sorry for the delay. |
|
is this one still alive? 🙂 |
|
I don't think this PR is, but I started to implement myself and after looking at the rules source code for like half an hour I'm pretty sure its super broken and needs re-implementing (i.e. its not checking for expects using Based on the tests I also think that this change is actually not quite what the OP wanted, but its hard to know for sure because they didn't give a code example in the issue 🤔 |
TODO
no-standalone-expectFixes #993