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

Lens tests #366

Merged
merged 21 commits into from
Mar 10, 2023
Merged

Lens tests #366

merged 21 commits into from
Mar 10, 2023

Conversation

nico-olivares
Copy link
Member

-Added one missing test and organized the rest.
-Please pay attention to a group of tests that are under the domLens object. I named those domLens.hover, domLens.focus, etc. I'm not sure about that so please comment.

@nico-olivares nico-olivares requested review from daedalus28 and a team August 29, 2022 18:20
@nico-olivares nico-olivares self-assigned this Aug 29, 2022
@nico-olivares nico-olivares marked this pull request as draft August 29, 2022 18:22
@decrapifier
Copy link
Contributor

decrapifier commented Aug 29, 2022

Warnings
⚠️ The README has not been updated. Please update the README.
⚠️

Branch being merged does not follow Git Flow

Messages
📖 Could not find any browser results.
📖 You reduced the total lines of code! Awesome! 👍

Generated by 🚫 dangerJS against 9325cc4

@nico-olivares nico-olivares marked this pull request as ready for review August 29, 2022 18:37
let object = { a: 1 }
let lens = F.lensOf(object)
lens.a.set(8)
expect(lens.a.get()).to.deep.equal(8)
Copy link
Member

@stellarhoof stellarhoof Aug 30, 2022

Choose a reason for hiding this comment

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

Deep equal makes more sense for objects with more than one level of nesting. It's not necessary for primitives.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you @stellarhoof. I fixed it.

@nico-olivares
Copy link
Member Author

@daedalus28 The branch is up to date and the tests are valid. The PR is ready for revision.

CHANGELOG.md Outdated
@@ -40,6 +44,7 @@
- Library issue resolution, two libraries where auto-updating minor versions and no longer compatible in those versions.
- Added ignore for yarn as NPM is being used currently
- Added versioning of package-lock files to resolve dependency issue.
> > > > > > > master
Copy link
Member

Choose a reason for hiding this comment

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

Leftover

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 find, thank you.

@@ -242,25 +205,24 @@ describe("Lens Functions", () => {
})
})
describe("domLens", () => {
it("value", () => {
it("domLens.value", () => {
Copy link
Member

Choose a reason for hiding this comment

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

No need for domLens. since the group is already named domLens

Copy link
Member Author

@nico-olivares nico-olivares Mar 10, 2023

Choose a reason for hiding this comment

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

@stellarhoof The it(string, ) has to match the title that appears in the docs. That's how they get matched. In the docs we have F.domLens.value and more, so the it has to be it('domLens.value', ...). Even so, sometimes the test is not found and hence there's no example for a function. At least that's how it was explained to me.

Copy link
Member

Choose a reason for hiding this comment

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

Got it

@nico-olivares nico-olivares merged commit 77761b2 into master Mar 10, 2023
@nico-olivares nico-olivares deleted the lensTests branch March 10, 2023 22:29
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

Successfully merging this pull request may close these issues.

None yet

3 participants