Skip to content

resolved test conflict #241

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

Merged
merged 1 commit into from
May 29, 2021
Merged

resolved test conflict #241

merged 1 commit into from
May 29, 2021

Conversation

shades-7
Copy link
Contributor

Description

Resolved the conflict arising in testing due to changing Link tag with achor tag.

Fixes #239

Type of Change:

  • Code

Code/Quality Assurance Only

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Screenshot from 2021-04-27 13-26-51

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials

Code/Quality Assurance Only

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works

@shades-7
Copy link
Contributor Author

@Rahulm2310 @mtreacy002 have a look please

@mtreacy002
Copy link
Member

mtreacy002 commented Apr 28, 2021

@shades-7 , all 3 tests are passing which is what we want. @jalajcodes and @Rahulm2310 , can we fix the warnings as a separate issues or you prefer to tackle it here?

Screenshots of the error warnings
image
image

@jalajcodes
Copy link
Member

jalajcodes commented Apr 28, 2021

@mtreacy002

can't perform a react state update on an uncomunted component...
This will require changing some things in the register component's code.

An update to PersonalDetails inside test was not wrapped inside a act().....
This usually indicates that we didn't test everything in the component. To fix it we'll have to figure out where in the code it is coming from.

If @shades-7 is up for it, let's fix these here otherwise we can always create another issue.

@Rahulm2310
Copy link
Contributor

@mtreacy002 @jalajcodes @shades-7 I agree with Jalaj. It would be better to fix all the warnings here itself.

@shades-7
Copy link
Contributor Author

I would love to solve this, but since I have no experience in testing, if you all can assist it will be great.🙂

@jalajcodes
Copy link
Member

I would love to solve this, but since I have no experience in testing, if you all can assist it will be great.🙂

Sure, that's why we are here. Now do you have an idea on how to tackle the first warning or do you need a hint?

@jalajcodes jalajcodes added Open Source Hack Status: Needs Review PR needs an additional review or a maintainer's review. labels Apr 28, 2021
@shades-7
Copy link
Contributor Author

shades-7 commented Apr 28, 2021

I would love to solve this, but since I have no experience in testing, if you all can assist it will be great.🙂

Sure, that's why we are here. Now do you have an idea on how to tackle the first warning or do you need a hint?

I will need hint @jalajcodes

@devkapilbansal
Copy link
Member

devkapilbansal commented May 18, 2021

I would love to solve this, but since I have no experience in testing, if you all can assist it will be great.slightly_smiling_face

Sure, that's why we are here. Now do you have an idea on how to tackle the first warning or do you need a hint?

I will need hint @jalajcodes

Pinging @jalajcodes @mtreacy002 again

@mtreacy002
Copy link
Member

@shades-7 and @devkapilbansal , I'm still new with React so I might not be able to answer this correctly. I'm assuming that @jalajcodes suggesting to add new test to be written/adjusted to match the changes in the personal details page. @jalajcodes or @Rahulm2310 perhaps can confirm this 😉

@mtreacy002
Copy link
Member

mtreacy002 commented May 23, 2021

@devkapilbansal and @shades-7, IMO we can approve and merge this PR as it is since the actual issue (failing tests) has been addressed. We can address the warnings issue on a separate PR because it is more important to make sure the next PR that comes after this will pass the tests. Let me know what you think.

@devkapilbansal
Copy link
Member

I agree with you @mtreacy002
We can discuss about this in a new issue

Copy link
Member

@mtreacy002 mtreacy002 left a comment

Choose a reason for hiding this comment

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

Approving this PR now. Will open a separate issue to tackle warnings 😉 . Thank you @shades-7 for your contribution to the project 🎉 🎉 🎉 .

@mtreacy002 mtreacy002 merged commit 07acdb6 into anitab-org:develop May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Open Source Hack Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Test failed after Link tag is used to replace anchor tag in Register.jsx and Login.jsx
5 participants