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

New tests #89

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

New tests #89

wants to merge 6 commits into from

Conversation

jsodicoff
Copy link
Collaborator

Hello again! I'm putting the new test file, which should have several more cases than the original, and the down sampled test object in this pull request. Currently, one error occurs in object subsetting and conversion but my other pull request handles that specific issue.

@vkozareva
Copy link
Collaborator

vkozareva commented Aug 15, 2019

Hey @jsodicoff !
Some notes on this PR:

  • Looks like some of the tests aren't passing because of changes introduced into master since you started writing them -- you'll have to update those to reflect the latest code.
  • The plotting tests in particular have a lot of incorrect values which need to be updated.
  • There's no real need to have two sets of tests on PBMC datasets of different sizes (so we should get rid of either test_smaller.R or the two existing test files (test_preprocessing.R and test_post_factorization.R).
  • I notice you've added tests for a feature that you've put in a different branch (for the dr.coords slot). We should keep related unit tests and features together in the same branch so that there is never a break in functionality if we merge one PR but not another.

Unfortunately we can't merge a PR until the CI build succeeds, so these things will need to be addressed -- the Travis CI log (linked below) should help you determine which tests are failing! If you need to move commits from one branch to another, you can use git cherry-pick.

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.

2 participants