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

removed rejection and unnecessary test cases as well #70

Merged
merged 5 commits into from
Oct 28, 2016

Conversation

harshkothari410
Copy link
Contributor

  • Fixed broken test
  • Removed all rejection error except one
  • Removed unnecessary testcases / duplication test cases

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 83.201% when pulling 6a3178d on removerejection into eb5ae26 on master.

@harshkothari410
Copy link
Contributor Author

Unhandled rejection SequelizeForeignKeyConstraintError: insert or update on table "Subjectsancestors" violates foreign key constraint "Subjectsancestors_SubjectId_fkey"

Rejection is remaining. I am doing research for that. This is happening from sequelize file.

@@ -47,7 +47,8 @@ function augmentSampleWithSubjectInfo(seq, inst) {
inst.dataValues.subject = sub;

// adding absolutePath to sample instance
inst.dataValues.absolutePath = sub.absolutePath;
if (sub)
Copy link
Contributor

Choose a reason for hiding this comment

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

please remember to run gulp style before submitting pr

we enclose all ifs--even single-line blocks--in curly braces

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 83.289% when pulling a4026ba on removerejection into eb5ae26 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 83.093% when pulling 9ee5baf on removerejection into 85e8e2e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 83.093% when pulling d6f0955 on removerejection into 95a65ed on master.

@iamigo
Copy link
Contributor

iamigo commented Oct 26, 2016

i'd like one more review on these changes, @pallavi2209 and/or @shriramshankar, thx!

.equal('geolocation');
done();
});
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing these 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.

Those test cases are redundant . Its already there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. Also, the code coverage has decreased, any ideas why?

Copy link
Contributor Author

@harshkothari410 harshkothari410 Oct 28, 2016

Choose a reason for hiding this comment

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

It was increased initially but due to some PR that is merged in master and then master merged it to this branch then it is decreased.

@shriramshankar shriramshankar merged commit e12f31e into master Oct 28, 2016
@iamigo iamigo deleted the removerejection branch October 28, 2016 20:47
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

5 participants