Skip to content

test(role-helpers): improve coverage of logRoles fn#1248

Open
naorpeled wants to merge 8 commits intotesting-library:mainfrom
naorpeled:fix/role-helpers/make-hidden-great-again
Open

test(role-helpers): improve coverage of logRoles fn#1248
naorpeled wants to merge 8 commits intotesting-library:mainfrom
naorpeled:fix/role-helpers/make-hidden-great-again

Conversation

@naorpeled
Copy link
Contributor

@naorpeled naorpeled commented Aug 5, 2023 •
edited
Loading

What:

Increased the tests coverage of the logRoles and prettyRoles functions in the role helpers file,
at the moment cases with hidden = true option were not covered.

Why:

To further understand the scope of #1201 I've added these tests, I think it's good to have them in the repo.

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • TypeScript definitions updated
  • Ready to be merged

@naorpeled naorpeled changed the title feat(role-helpers): add some tests and refine TS types" feat(role-helpers): add some tests and refine TS types Aug 5, 2023
MatanBobi
MatanBobi reviewed Aug 6, 2023
View reviewed changes
Copy link
Member

@MatanBobi MatanBobi left a comment

Choose a reason for hiding this comment

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

Thanks for this one @naorpeled!
Left a minor comment.

@naorpeled naorpeled requested a review from MatanBobi August 6, 2023 10:14
MatanBobi
MatanBobi reviewed Aug 6, 2023
View reviewed changes
timdeschryver
timdeschryver reviewed Aug 11, 2023
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @naorpeled.
Because this PR contains 2 changes (types, and additional tests) I think it would be better to also create 2 focused PR.
This makes it easier to follow, and we have more control over the releases/history.

@naorpeled
Copy link
Contributor Author

naorpeled commented Aug 11, 2023

Thanks for the PR @naorpeled.
Because this PR contains 2 changes (types, and additional tests) I think it would be better to also create 2 focused PR.
This makes it easier to follow, and we have more control over the releases/history.

Fair point.
Will do.

@naorpeled
Copy link
Contributor Author

naorpeled commented Aug 11, 2023 •
edited
Loading

@timdeschryver thanks for the great feedback.
I'm heading off to sleep, will make the adjustments tomorrow 🙏

@naorpeled
Copy link
Contributor Author

naorpeled commented Aug 12, 2023

Hey @timdeschryver,
I've updated the PR based on your CR.
Also opened this PR for the type defs.

@naorpeled naorpeled changed the title feat(role-helpers): add some tests and refine TS types test(role-helpers): improve coverage of logRoles fn Aug 12, 2023
timdeschryver
timdeschryver approved these changes Sep 9, 2023
View reviewed changes
Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

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

LGTM.
Sorry, I missed the notification

@naorpeled
Copy link
Contributor Author

naorpeled commented Sep 9, 2023

LGTM.
Sorry, I missed the notification

All good :)
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timdeschryver timdeschryver timdeschryver approved these changes

@MatanBobi MatanBobi Awaiting requested review from MatanBobi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@naorpeled @MatanBobi @timdeschryver