Skip to content

New regex-based snapshot parsing breaks when using = in the snapshot name #122

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

Closed
jurriaan opened this issue Nov 23, 2022 · 5 comments
Closed

Comments

@jurriaan
Copy link

As seen here the name part of the regex is non-greedy, so it stops matching when it encounters the first =:

String regex = "^(?<name>.*?)(\\[(?<scenario>.*)\\])?=(?<header>\\{.*\\})?(?<snapshot>(.*)$)";

We currently have snapshot names like:

foo.bar.MailBuilderSpec.generates a correct template when recipientIsCustomer = true

Which worked fine on previous versions, but on v4 this is broken and our tests fail, which means this prevents us from upgrading to the latest version.

@jackmatt2
Copy link
Member

jackmatt2 commented Nov 25, 2022

Yeah - I was worried the introduction of the REGEX might break someone 😟 .

The REGEX needs to remain NON-Greedy or else it will eat into the actual snapshot. I might need to restrict the use of the = character inside the snapshot name.

A Few options:

  1. Error out with a nice message when an "=" is found in the snapshot name
  2. Change it to something like "equals" and leave a warning message

In any case I think you will need to manually replace the "=" with something else unfortunately.

@jackmatt2
Copy link
Member

'[', ']' and '=' are no longer allowed in snapshot names and scenario names.

@koenpunt
Copy link

Seems a bit odd to add restrictions for names, and requiring people using this library to change how they name there snapshots because a the parser can't handle it? Even more so because this worked just fine before adding this feature.

@jackmatt2
Copy link
Member

Note that this was a major version bump - 3.X to 4.X.

There were many breaking changes in the 4.X release - especially for people using custom Serializers, Comparators & Reporters that go far beyond the above mechanical refactor.

Breaking changes are commonly accepted in major version bumps as long as there is an upgrade path for existing users.

In the end there were conflicting concerns. People using custom logic were not happy with how difficult it was to extract information about the snapshot from the raw string. The introduction of the REGEX gives structure to the snapshot and thus allows this information to be extracted.

The introduction of "headers" allows further metadata to be injected into the snapshot for people using custom logic. I tended to agree that it was too difficult previously and these changes were necessary.

Again - it's just a mechanical refactor "find & replace" for existing users to update to version 4.0.

@jackmatt2
Copy link
Member

Seems a bit odd to add restrictions for names, and requiring people using this library to change how they name there snapshots because a the parser can't handle it? Even more so because this worked just fine before adding this feature.

I think this restriction can be relaxed once encoding/decoding is implemented. That way you will be able to start using these characters again - the only difference is they will be escaped in the snapshot output.

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

No branches or pull requests

3 participants