Skip to content

Sai Sandeep taking over for Ramya unit test : Reports/PeopleTableDetails#2102

Open
scoopstroop92 wants to merge 8 commits intodevelopmentfrom
ramya-unit-test-report-people-table-details
Open

Sai Sandeep taking over for Ramya unit test : Reports/PeopleTableDetails#2102
scoopstroop92 wants to merge 8 commits intodevelopmentfrom
ramya-unit-test-report-people-table-details

Conversation

@scoopstroop92
Copy link
Copy Markdown

@scoopstroop92 scoopstroop92 commented Mar 31, 2024

Description

Unit test for src/components/Reports/PeopleTableDetails

Main changes explained:

  • New file
    • PeopleTableDetails.test.js : unit test cases
  • Existing file file
    • PeopleTableDetails.test.js : Added test id

Test cases

image

How to test:

  1. check into current branch
  2. do npm test PeopleTableDetails.test.js
  3. check if all the test cases pass

Screenshots or videos of changes:

image

Note :

Kindly take a look at the test cases and suggest if more validations can be added

Copy link
Copy Markdown
Contributor

@aaryaneil aaryaneil left a comment

Choose a reason for hiding this comment

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

Reviewed your PR and all the test cases passed but there were a lot of warnings generated, please look into it. Good work.
Screenshot 2024-04-20 at 7 24 09 AM
Screenshot 2024-04-20 at 7 24 22 AM

@scoopstroop92
Copy link
Copy Markdown
Author

Reviewed your PR and all the test cases passed but there were a lot of warnings generated, please look into it. Good work. Screenshot 2024-04-20 at 7 24 09 AM Screenshot 2024-04-20 at 7 24 22 AM

The warning stems from the component's code design so that is not something I can address in the unit test

Copy link
Copy Markdown

@Tiantian-C Tiantian-C left a comment

Choose a reason for hiding this comment

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

All the tests have passed.
Screenshot 2024-04-20 at 4 05 37 PM

Copy link
Copy Markdown
Contributor

@metaphor987 metaphor987 left a comment

Choose a reason for hiding this comment

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

Hi Ramya, all tests have passed on my side but there are some warnings, and it would be best to check them. Here is the screenshot showing that all tests have passed:
Screenshot 2024-04-20 at 10 21 09 PM

and here are screenshots showing some of the warnings:
Screenshot 2024-04-20 at 10 19 44 PM
Screenshot 2024-04-20 at 10 20 44 PM

Thank you!

Copy link
Copy Markdown
Contributor

@jennZhang93 jennZhang93 left a comment

Choose a reason for hiding this comment

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

Testcases passed but here are some console log error that showed up.

Screenshot 2024-04-20 at 11 18 38 PM Screenshot 2024-04-20 at 11 18 47 PM Screenshot 2024-04-20 at 11 19 00 PM Screenshot 2024-04-20 at 11 19 08 PM Screenshot 2024-04-20 at 11 19 15 PM Screenshot 2024-04-20 at 11 19 22 PM

@scoopstroop92
Copy link
Copy Markdown
Author

@metaphor987 @aaryaneil I have fixed the console warnings. Now you shouldn't see any warnings except for warning due to the main component logic

@Chuehleo Chuehleo added High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible and removed High Priority - Please Review First labels May 1, 2024
Copy link
Copy Markdown
Contributor

@cgomezhub cgomezhub left a comment

Choose a reason for hiding this comment

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

Good Job,!,
All test has passed . Just to avoid warning you need to add in line 221:
key={value._id} : <NewModal key={value._id} header="Task info" trigger={() => renderFilteredTask(value)}>

do it and please let me know.

image

@cgomezhub cgomezhub self-requested a review May 10, 2024 14:32
Copy link
Copy Markdown
Contributor

@KaushikMreddy KaushikMreddy left a comment

Choose a reason for hiding this comment

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

Hi Ramya,

All test cases have passed with 1 warning, pasting that warning here
image
image

Good work!

dkt6

This comment was marked as duplicate.

@dkt6 dkt6 closed this May 13, 2024
@dkt6 dkt6 reopened this May 13, 2024
Copy link
Copy Markdown
Contributor

@dkt6 dkt6 left a comment

Choose a reason for hiding this comment

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

As others have already mentioned in their comments, there is a warning to add a unique ID. Thank you for your great work!

2102

@ZhangPeizhou
Copy link
Copy Markdown
Contributor

ZhangPeizhou commented May 14, 2024

image
Pass all tests, nice work!

@scoopstroop92
Copy link
Copy Markdown
Author

@ZhangPeizhou @cgomezhub @dkn6 my understanding is the warning stems from the main component logic. I am not really sure if i can make that changes as part of this PR . I beleive it should be addressed in a separate PR altogether as the component needs to functionally tested after making any changes. Let me know your thoughts

Comment on lines 219 to 220
<div className="people-table">
{filteredTasks.map(value => (
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ramyaram2092
From my understanding, simply assigning a unique key to the list of items here should be enough, and I don't think this change will have any negative impact. Whether to handle this in a separate PR is up to you, so I approve this PR. Thank you for your great work!

Copy link
Copy Markdown

@Sandhya1236 Sandhya1236 left a comment

Choose a reason for hiding this comment

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

I checked out the current branch, ran npm test PeopleTableDetails.test.js, and verified that all the test cases passed. All steps were followed successfully, confirming that the functionality covered by the PeopleTableDetails.test.js file is working as intended.

image

Copy link
Copy Markdown
Contributor

@Kavil-Jain-514 Kavil-Jain-514 left a comment

Choose a reason for hiding this comment

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

All test cases have passed. However, there is a warning for a unique key. Each element in an array rendered by a map must have a unique key prop.
image
image

You can replace the line numbers 221 to 229 in the PeopleTableDetails.jsx with this code:
{filteredTasks.map(value => (
<NewModal key={value._id} header="Task info" trigger={() => renderFilteredTask(value)}>

Why This Task is important

<textarea className="rectangle" type="text" value={value.whyInfo} readOnly />
Design Intent

<textarea className="rectangle" type="text" value={value.intentInfo} readOnly />
End State

<textarea className="rectangle" type="text" value={value.endstateInfo} readOnly />

))}

Also, you can make your code better by removing extra stuff like.
Line no. 61, remove const simple = [ ] as it is never used.
Line no. 64 to 72, use return instead of an if and return true.

Copy link
Copy Markdown
Contributor

@JatinAgrawal94 JatinAgrawal94 left a comment

Choose a reason for hiding this comment

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

All the tests pass, but with a warning.
The NewModal Component doesn't have a key prop on line 221 in PeopleTableDetails.jsx. Rest everything works fine.
Good Job.
2102

@akshit0211
Copy link
Copy Markdown
Contributor

All test cases passed but with warning. I saw your previous comment so I'm approving this.

test cases passed

@scoopstroop92
Copy link
Copy Markdown
Author

the warning stems from the main component logic. I am not really sure if i can make that changes as part of this PR . I beleive it should be addressed in a separate PR a

the warning stems from the main component logic . I beleive it is better to make the changes as a seperate PR. Let me know your thoughts @JatinAgrawal94 @Kavil-Jain-514

Copy link
Copy Markdown
Contributor

@deepthikannan deepthikannan left a comment

Choose a reason for hiding this comment

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

image
All test cases passed.

Copy link
Copy Markdown

@Vigneshwar-Muriki Vigneshwar-Muriki left a comment

Choose a reason for hiding this comment

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

Tested this PR as mentioned in the instructions and its working as intended. All test cases have passed.
Frontend PR #2102

Copy link
Copy Markdown

@AnujVakil AnujVakil left a comment

Choose a reason for hiding this comment

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

I have tested this PR and all test cases are passed
PR #2102_1

@ZhangPeizhou
Copy link
Copy Markdown
Contributor

all tests are passed, well done!
PR#2102

Copy link
Copy Markdown
Contributor

@Shadhrush5 Shadhrush5 left a comment

Choose a reason for hiding this comment

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

Hi, I have reviewed PR 2102. I followed the steps as mentioned and see that all the test cases execute and is working as expected. Thank you for the great work.

Screenshot 2024-07-14 at 8 08 29 AM

Copy link
Copy Markdown

@vamshik2024 vamshik2024 left a comment

Choose a reason for hiding this comment

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

Reviewed the PR, test cases passed but could see couple of warnings related to keys while rendering PeopleTableDetails

image

Good work!

@souravwalke
Copy link
Copy Markdown

I have reviewed PR 2102 and all the test cases executed without any fail.

Screenshot 2024-07-27 at 6 07 31 PM

@Gmon-k
Copy link
Copy Markdown
Contributor

Gmon-k commented Aug 7, 2024

I have tested from my side and all the test cases has passed. It looks fine.
2

@one-community one-community added Moved to Final Review and removed High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible labels Aug 11, 2024
@navya9989
Copy link
Copy Markdown

Hi, I have tested this PR and all test suits have passed, Good worl!
@
PR # 2102

@SatyashanthiT
Copy link
Copy Markdown
Contributor

I reviewed and tested PR for the Reports/PeopleTableDetails unit tests. Verified that all test cases outlined in PeopleTableDetails.test.js passed, confirming the expected behavior for the component.
image

@geetamatkar
Copy link
Copy Markdown
Contributor

Tested this PR and all the test cases have passed.

Screenshot 2024-10-25 at 8 09 32 PM

@one-community one-community changed the title Ramya unit test : Reports/PeopleTableDetails Sai Sandeep taking over for Ramya unit test : Reports/PeopleTableDetails Mar 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.