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

[MRG] assert_true which checks for equality replaced with assert_equal #1406

Merged
merged 4 commits into from Jan 30, 2018

Conversation

Projects
None yet
3 participants
@thechargedneutron
Contributor

thechargedneutron commented Jan 26, 2018

Fixes #1387

thechargedneutron added some commits Jan 26, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jan 26, 2018

Codecov Report

Merging #1406 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1406      +/-   ##
==========================================
+ Coverage   87.25%   87.27%   +0.01%     
==========================================
  Files         229      229              
  Lines       29588    29588              
  Branches     3170     3170              
==========================================
+ Hits        25817    25822       +5     
+ Misses       3042     3039       -3     
+ Partials      729      727       -2
Impacted Files Coverage Δ
dipy/workflows/tests/test_masking.py 91.66% <100%> (ø) ⬆️
dipy/workflows/tests/test_reconst_csa_csd.py 96.07% <100%> (ø) ⬆️
dipy/segment/tests/test_metric.py 97.5% <100%> (ø) ⬆️
dipy/tracking/tests/test_streamline.py 98.16% <100%> (ø) ⬆️
dipy/segment/tests/test_clustering.py 99.38% <100%> (ø) ⬆️
dipy/workflows/tests/test_reconst_dti.py 95.08% <100%> (ø) ⬆️
dipy/core/graph.py 75% <0%> (+1.19%) ⬆️
dipy/reconst/forecast.py 92.22% <0%> (+2.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2bbb25...fea9ddd. Read the comment docs.

@skoudoro

Looks good to me.

Can you change some assert_true on the following line too?

@thechargedneutron

This comment has been minimized.

Contributor

thechargedneutron commented Jan 28, 2018

@skoudoro Thanks for the review.
However, test_utils.py contains assert_true(x is y) and not assert_true(x==y). Are these two equivalent in that context?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Jan 28, 2018

No, sorry, it seems we are testing the reference equality and not the value.
Thanks for pointing that @thechargedneutron.

LGTM, anyone for reviewing this PR before I merge ?

@skoudoro

This comment has been minimized.

Member

skoudoro commented Jan 30, 2018

@skoudoro skoudoro merged commit 5ae6eb2 into nipy:master Jan 30, 2018

3 checks passed

codecov/patch 100% of diff hit (target 87.25%)
Details
codecov/project 87.27% (+0.01%) compared to b2bbb25
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1406 from thechargedneutron/introduce_assert_…
…equal

[MRG] assert_true which checks for equality replaced with assert_equal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment