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

[test] Fix adapters code coverage #13969

Merged
merged 20 commits into from
Jul 26, 2024
Merged

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Jul 24, 2024

The #13608 PR revealed the codecov.yml was not valid. So the 100% requirement was not applied.

This PR fix the config file by moving ignore parameter, and add missing tests

There are still some remaining untested lines

The remaining lines

✅ Luxon

I don't see in which case the localised week day could not be defined

Warning

This is probably untestable.
The localeWeekNumber is defined only with latest luxon release, where the support for it was added.
Added an istanbul ignore next to omit it from coverage.

return value.localWeekNumber ?? value.weekNumber;

✅ Dayjs

Technically we could add a test "it should not display the error message in production"

Tip

Not sure it is worth it.
Added an istanbul ignore next to omit it from coverage.

if (process.env.NODE_ENV !== 'production') {

✅ DateFns (v2,v3, v2 jalaali, v3 jalaali)

✅ Major version assertions

Tip

Added an istanbul ignore next to omit it from coverage.

if (typeof addDays !== 'function') {
throw new Error(
[
`MUI: The \`date-fns\` package v2.x is not compatible with this adapter.`,
'Please, install v3.x of the package or use the `AdapterDateFns` instead.',
].join('\n'),
);
}

if (!longFormatters) {
throw new Error(
'MUI: The minimum supported `date-fns` package version compatible with this adapter is `3.2.x`.',
);
}

✅ Locale code fallback

Warning

I have removed the fallback, because locale and it's code is always defined: cd2412f.
As listed below: if an invalid locale is passed without a code: that's a problem that I'm not sure we should be safeguarding against. 🤷

With the adapter having the following defaultize, the only way I found to have no locale.code is to pass an invalid locale string. But that make no sens because other methods will faill

locale: locale ?? defaultLocale,

Proposal

We could set the config file with threshold: 0% to just be sure we do not deteriorate this

@mui-bot
Copy link

mui-bot commented Jul 24, 2024

Deploy preview: https://deploy-preview-13969--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 181ff21

@alexfauquette alexfauquette added test component: pickers This is the name of the generic UI component, not the React module! labels Jul 24, 2024
@LukasTy
Copy link
Member

LukasTy commented Jul 25, 2024

@alexfauquette Thank you for taking care of it. 🙏
I'll have a look at improving things here. 👍

@flaviendelangle
Copy link
Member

This is probably untestable.

Yeah it is 😬
You can add:

/* istanbul ignore next */

LukasTy proposal is to reduce the threshold slightly (to 99.5%) to leave some slack for those edge cases, that we do not want to hack some extra behavior into properly increasing the coverage.

I would personally use istanbul ignore next instead of decreasing the threshold. At least we decide which lines are edge cases.

@LukasTy
Copy link
Member

LukasTy commented Jul 25, 2024

I would personally use istanbul ignore next instead of decreasing the threshold. At least we decide which lines are edge cases.

Thanks for reminding about this. 👍
Would you also revert the changes I've done with the addDays and longFormatters moving? 🤔

].join('\n'),
);
/* istanbul ignore next */
if (process.env.NODE_ENV !== 'production') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reverted the previous changes and added this check to avoid bundling these warning in production.
Do you think it is worth it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for me

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the warnOnce, it's definitely worth it

@LukasTy LukasTy requested a review from arthurbalduini July 25, 2024 14:19
@LukasTy
Copy link
Member

LukasTy commented Jul 25, 2024

Well, given that I've done part of the work, I'm leaving the approval step for @flaviendelangle. 🙈

Copy link
Member Author

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't approve since I'm the author, but this PR looks good to me 👍

For the remaining line not tested, probably skipping it is the good way

https://app.codecov.io/gh/mui/mui-x/pull/13969/blob/packages/x-date-pickers/src/AdapterDayjs/AdapterDayjs.ts#L281

@LukasTy LukasTy requested a review from flaviendelangle July 26, 2024 05:47
@LukasTy LukasTy merged commit 6e7db26 into mui:master Jul 26, 2024
17 checks passed
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants