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 for Get Api Updated #97

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Test for Get Api Updated #97

wants to merge 4 commits into from

Conversation

Sukriti-sood
Copy link
Contributor

@Sukriti-sood Sukriti-sood commented May 3, 2021

Fixes #82
Fixes #107
@Manvityagi
I have added test but , facing a problem , I am not getting idea how to stub chained mongoose Operation , I tried but only One is working (skip or limit ) not both.

@Manvityagi
Copy link
Member

Manvityagi commented May 8, 2021

Fixes #82

@Manvityagi
I have added test but , facing a problem , I am not getting idea how to stub chained mongoose Operation , I tried but only One is working (skip or limit ) not both.

@Sukriti-sood
Try something like this, it should work

 try {
        result.results = await this.opportunity.find(
          {
            /* Everything*/
          },
          {
            /* No constraints */
          },
          {
            skip: skip,
            limit: limit,
          }
        );

@Manvityagi
Copy link
Member

Also, pull latest code before making next commit @Sukriti-sood

@Sukriti-sood
Copy link
Contributor Author

@Manvityagi @abdus Added Test , Please Review.

@abdus abdus requested a review from Manvityagi May 9, 2021 08:45
@Sukriti-sood
Copy link
Contributor Author

@Manvityagi Please Review PR

@ay2306
Copy link
Contributor

ay2306 commented May 24, 2021

You are writing unit tests, you should not change the source code. Please correct it. And your Travis Build is failing, look into it as well.

@Sukriti-sood
Copy link
Contributor Author

Sukriti-sood commented May 24, 2021

You are writing unit tests, you should not change the source code. Please correct it. And your Travis Build is failing, look into it as well.

@ay2306 Actually Logic of Pagination Helper Was Wrong I just Corrected it.

@ay2306
Copy link
Contributor

ay2306 commented May 24, 2021

Please raise an issue, explain why is it wrong? Make PR to that. Cannot review code without context.

@Sukriti-sood
Copy link
Contributor Author

Sukriti-sood commented May 24, 2021

Please raise an issue, explain why is it wrong? Make PR to that. Cannot review code without context.

@ay2306 I have fixed the error, Can I explain it here , rather than making new issue which is not needed as changes already made I guess .

In the pagination helper variable like skip , start index , begin was not needed and
if (numDocs >= limit) { totalPages = Math.floor(numDocs / limit) + (numDocs % limit); }
This logic for total pages was also wrong.

Is making an Issue Needed ? I will make If its that necessary

@ay2306
Copy link
Contributor

ay2306 commented May 24, 2021

Yes, create a new issue, issues should be tracked properly, link it in this PR, and indent properly so that the diff checker shows the changes only. You have added extra characters in many places and it is difficult to see what changes have you made. Please revert all changes shown in diff which you haven't actually changed.

@Sukriti-sood
Copy link
Contributor Author

Sukriti-sood commented May 24, 2021

@ay2306 Why are u asking for revert, all the changes mentioned in this PR are only made by me. and that extra characters are due to prettifying code .

@ay2306
Copy link
Contributor

ay2306 commented May 24, 2021

Yeah, dont beautify. Have to read complete file to find out changes.

@Sukriti-sood
Copy link
Contributor Author

Yeah, dont beautify. Have to read complete file to find out changes.

Are you Mentor Here ? As Mentor/PA of This Project believes in Clean Code as much I know.

@ay2306
Copy link
Contributor

ay2306 commented May 24, 2021

Dude, I was asked to review this PR. The issue is not with beautify, the issue is that since you beautified the whole file, it is difficult to see where are the changes. Beautify when we are sure that nothing new is added but I cannot say okay to a file without reading what is changed and it is difficult to do so because the whole file has been beautified. Imagine you are reviewing your PR and you see most of the diff say a file is changed because a tab has been added, but there are some changes as well so now you have to go line by line matching if this has been changed or not.

@Sukriti-sood
Copy link
Contributor Author

@ay2306 I have already Opened Issue #107 for Pagination Helper

And For File Changed , There are Only 7 files Changed and Only 2 files are extra having with no code change ,

Still I will remove those 2 files in upcoming commit .

Anything Else needed.

@ay2306
Copy link
Contributor

ay2306 commented May 24, 2021

In pagination.js, remove extra space so they are not treated as changed in diff

@Sukriti-sood
Copy link
Contributor Author

Sukriti-sood commented May 24, 2021

@ay2306 I have removed extra File , Please Review it

@ay2306
Copy link
Contributor

ay2306 commented May 24, 2021

You have added extra space, it is making helper/pagination.js like that
image
image
Remove beautification and just keep your changes there.

@Sukriti-sood
Copy link
Contributor Author

@ay2306 Sir , where is extra space , I am not getting your words, Please explain me.

@ay2306
Copy link
Contributor

ay2306 commented May 24, 2021

For example in helper/pagination.js
Earlier there was this line
image
Now that line has become this
image
Like this, there are several lines that are not changed at all but are shown as changed due to added indentation. This is not required and makes reviewing difficult because I have to compare each line one by one to see if this is something you added or this is only a tab. This defeats the whole purpose of having a diff.

Even if we review it, there will be many cases when we may need to come back to this PR in the future to see what changes were made. We need the changes to be reviewed by any developer as quickly as possible.

@Sukriti-sood
Copy link
Contributor Author

@ay2306 Sir Please have a look now, Is this fine or still any changes needed..?

Copy link
Contributor

@ay2306 ay2306 left a comment

Choose a reason for hiding this comment

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

Please look at the code review comments.

helpers/pagination.js Show resolved Hide resolved
// If the page entered exceeds the Max allowed value(totalPages)
if (page > totalPages) {
let maxAllowedPages = totalPages;
result.results = 'The maximum allowed pages are ' + maxAllowedPages;
return result;
}

// StartIndex should be atleast 0 and for the min val of Page should be 1
if (startIndex < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove these checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Purpose this has been updated in my code.

helpers/pagination.js Outdated Show resolved Hide resolved
@@ -22,4 +22,6 @@ let stubValue = {
updatedAt: faker.date.past(),
};

export default stubValue;
let fakedataArray = [stubValue, stubValue, stubValue, stubValue];
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this generate an array of 4 same stubValues since stubValue is generated only once? Please note stubValue is a variable and not a method, so here it will be same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ay2306 This is generated to test the cases like if limit is given so whats the output , and related to page query example :-
if page is 2 and limit is 2 resultant array will be of length 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if limit is more than 4? You should add test for that as well. And wont it be better if we convert it to a function so we can call it whenever we want 6 or 10 times or even more.

Copy link
Contributor Author

@Sukriti-sood Sukriti-sood May 24, 2021

Choose a reason for hiding this comment

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

Ya it will be nyc , @ay2306 Currently I am facing issue for stubbing both mongoose operation find, skip and limit , I need help in this , .

expect(result.results).to.equal(
'The Parameters cannot be Negative or Zero'
);
});
});

Copy link
Contributor

@ay2306 ay2306 May 24, 2021

Choose a reason for hiding this comment

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

Please add tests for all situations where it will throw an error as well, like non-positive limit, non-positive page, cover more edge cases. These are only positive tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ook I will do.

type: stubValue.opportunityType,
});
})).results[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Add more assertions like expected length, if all entries are correct or not.

@Sukriti-sood Sukriti-sood requested a review from ay2306 May 24, 2021 15:49
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.

Bug in pagination helper code Update Tests of GET API
3 participants