refactor: email test cases

[ariskemper]

@fabian-hiller Here is my proposal for email test cases. I fixed regex so it supports
max 64 chars in local part of email value and 255 chars in domain part.

[netlify]

✅ Deploy Preview for valibot canceled.

Name	Link
🔨 Latest commit	28764fb
🔍 Latest deploy log	https://app.netlify.com/sites/valibot/deploys/656a0308d466370008c8477c

[fabian-hiller]

Thanks for continuing to Valibot creating this PR! Do you know if there is a way to group related tests? For example, tests to check valid values, tests to check invalid values, and so on.

[ariskemper]

@fabian-hiller We could wrap the tests in another describe for grouping. I was also thinking to create a helper function which takes as an argument validate._parse(value) and returns true or false.
Would be nicer to read test like

expect(isValidParse(validate._parse(value))).toBeTruthy();

or

expect(isValidParse(validate._parse(value))).toBeFalsy();

[fabian-hiller]

I think I like the grouping but I am not sure about the utility functions. I like that the tests show the raw code, but on the other hand I also see the advantage of making the code more readable and simple. If we were to create a helper function for this, I would choose the following API:

expect(isValidParse(validate, value)).toBeTruthy();
expect(isValidParse(validate, value)).toBeFalsy();

Do you have an idea for alternative names for the utility function? Maybe passes, validate or parse. I am looking for a word that makes the whole API more readable.

[ariskemper]

@fabian-hiller i also like tests showing raw code, but readable tests are also important so my suggestion is following:

const emailValidate = email();
expect(parse(emailValidate, value)).toBeTruthy();
expect(parse(emailValidate, value)).toBeFalsy();

[fabian-hiller]

Looks good to me. Feel free to implement this for some tests so we can compare. A disadvantage of this approach is that not everything can be covered with true and false and we still have to resort to raw code in some cases.

[ariskemper]

@fabian-hiller what do you think about this PR and parseValidation? If you agree, we could merge it and i refactor current mac and bic tests?

[fabian-hiller]

I will give you feedback today or in the next few days.

[ariskemper]

@fabian-hiller any update on this?

[fabian-hiller]

It's on my list. I have not forgotten. By the end of next week I will be working full time on Valibot for the next few weeks. I expect big progress on the source code and documentation.

[ariskemper]

@fabian-hiller i was thinking, maybe we could have some status in validation, which is setled, based on validation true, false, besides issues array of validation messages.

[fabian-hiller]

What is the advantage of such an status key in your eyes? So far I have consciously decided against it for the internal code because it was not necessary. For external use of a schema, for example with safeParse, such a status is returned with .success.

[fabian-hiller]

I like the more fine-grained tests with a separate message for each case. I also like the 3-layer system (the grouping) with the nested describe. I am not sure about the parseValidation function, because I think it is better not to add an abstraction here. Currently I tend not to use it. Also, instead of just testing .output, I think that I would prefer if we test the entire object returned by email. Feel free to give me feedback on this.

[fabian-hiller]

Thank you again for your contribution. I will try to review it the next days and give you feedback.

[fabian-hiller]

This PR looks good now. There is only one thing I would change. I think instead of just testing the output, we should test the whole object returned by ._parse. This makes the tests safer. What do you think?

// Old
test('should pass standard email', () => {
  const value = '[email protected]';
  expect(validate._parse(value).output).toBe(value);
});

// New
test('should pass standard email', () => {
  const value = '[email protected]';
  expect(validate._parse(value)).toEqual({ output: value });
});

[fabian-hiller]

Once this PR is merged, I recommend creating another PR for the tests of the string schema function, and then we can rework the tests of the entire library.

[ariskemper]

@fabian-hiller i agree to compare with output object.

[fabian-hiller]

Thanks for updating this PR! I will review it again next week and probably merge it.

[ariskemper]

@fabian-hiller please check PR #366 for string schema refactored tests.

[fabian-hiller]

Please have a look an review my changes. Note: I have removed the "should return custom error message" test because after the refactor of #397 we will pass the entire context to actionIssue. With "should contain the message argument" we ensure that the messages is part of the context.

[ariskemper]

@fabian-hiller it looks ok.

[fabian-hiller]

Thank you very much! Once i18n, the docs, and one or two other features are shipped, I will focus much more on unit testing.

[fabian-hiller]

Is .toMatchObject() less strict than .toEqual()? Why do you choose it here?

[fabian-hiller]

Thank you again for this PR Aris! I added your tests to the rewrite of Valibot in this commit.