feat(to-match-snapshot): show no-large-snapshots in js file #282
Conversation
benmonro
force-pushed
the
feature/toMatchSnapshot
branch
2 times, most recently
from
0c559f0
to
f135d33
Compare
benmonro
force-pushed
the
feature/toMatchSnapshot
branch
from
f135d33
to
f1b68c4
Compare
|
@SimenB can we get this merged? It is a breaking change since it's now reporting the long snapshot in the .js file instead of the .snap file. |
|
@SimenB pretty please? :) I think teams that have a large number of large snapshots are probably experiencing the same problem as me. They want to block new large snapshots going forward, but they have a large quantity of them that they need to grandfather in using |
| reportOnViolation(context, node); | ||
| if (isTestCase(node) || isDescribe(node)) { | ||
| const [firstArgument] = node.arguments; | ||
| titles.push(getStringValue(firstArgument)); |
There was a problem hiding this comment.
Does this work for
const x = 42;
test(`${x}`, () => {});?
There was a problem hiding this comment.
good question, I will look for that and test it.
There was a problem hiding this comment.
@jeysal so the short answer is no. The long answer is that i have a couple options. One is to treat variables in a template literal as regular expressions when trying to resolve which snapshot they are associated with (not great). Another is to fallback to reporting the error in the snap file. Curious if you have any other thoughts. Or if you know of a way to resolve the variables in the template literal so that the whole strong value can be reconstructed. At the moment, i'm thinking that it falls back to the .snap file for when it can't resolve it back to a .js file. do you think that would be enough to get this merged? otherwise i will have to fork because we desperately need this feature at Walmart Labs and the way it's implemented works for us as is.
| @@ -78,15 +79,77 @@ describe('no-large-snapshots', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describe('ExpressionStatement function', () => { | |||
| describe('toMatchSnapshot expression statement and call expressions', () => { | |||
| const testNode = { | |||
There was a problem hiding this comment.
Can we maybe use ESLint's parser here to write these nodes as source code template literals in the tests instead? Would keep the file from exploding in size
There was a problem hiding this comment.
Would also make it a lot easier to see if we have all cases we can think of covered with tests
There was a problem hiding this comment.
@jeysal added a test for snapshot in a describe, and multiple tests w/ mulitple snaps. let me know if you can think of others.
|
@jeysal thank you for the feedback, i will make sure all those things are addressed. Sorry for being impatient, we'd like to roll this into our code ASAP so would like to get it merged soon. we are bleeding large snapshots and i'd like to stop the hemorrhaging. :) |
|
@anescobar1991 what do you think about these changes? |
|
closing this PR since we were able to accomplish our goal of whitelisting existing big snapshots using #288 it is an interesting challenge though, just one that I don't have time to solve. :( |
|
I think this is an improvement - you want the error displayed where it's actionable, which is by the Are we missing something to keep us from landing this? I'd like feedback from the rule's author, but beyond that? |
|
@SimenB there's some big challenges around template literals with variables in them (ex describe.each). I'm not really convinced that those problems can be resolved without some consistent way of associating a test to a snapshot and Visa versa. Will re open but the examples that @jeysal brought up highlight the issue that prevent this from being merged |
|
Just adding my support to this proposal. Unfortunately, I don't even see the error raised on the snap file at this point. |
|
I'm going to close this for now as it seems to have gotten stale, and it sounds like there's no clear golden path yet on how to solve this issue. |
BREAKING CHANGE: this change will no longer report errors in the snap file, but instead report them on
.toMatchSnapshot()calls instead.Currently, when the
no-large-snapshotsrule runs, it highlights the problem in the .snap file. The problem is you can't useeslint-disable-next-linein those files as they are generated and will get wiped out whenever-uis used. This PR instead match the snapshot with thetoMatchSnapshot()call thus allowing eslint-disable-next-line. This is very useful for teams like ours which have a large number of large snapshots that we need to set to disable, while still blocking new large snapshots going forward.The change here will still parse the snap files, but will keep track of them and align them with the test that is calling toMatchSnapshot.
Resolves #281