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

add rabin min error test #5449

Merged
merged 4 commits into from
Sep 14, 2018
Merged

add rabin min error test #5449

merged 4 commits into from
Sep 14, 2018

Conversation

kjzz
Copy link
Contributor

@kjzz kjzz commented Sep 12, 2018

feature #5425

License: MIT
Signed-off-by: Kejie Zhang 601172892@qq.com

@kjzz kjzz requested a review from Kubuxu as a code owner September 12, 2018 06:15
@kjzz
Copy link
Contributor Author

kjzz commented Sep 12, 2018

@Stebalien after you update gx dep , i add tests about rabin min error.please help me review it ,Thx

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Thanks! @kevina actually updated the chunker dep yesterday so nothing should be blocking this.

ipfs add --chunker rabin-36-512-1024 mountdir/hello.txt >actual
'

test_expect_failure "ipfs add --chunker rabin-12-512-1024 failed" '
Copy link
Member

Choose a reason for hiding this comment

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

FYI, test_expect_failure is actually poorly named. It's for testing known bugs, not testing things that should fail. This should actually just be test_expect_code 1 add --chunker ...

@@ -156,6 +156,15 @@ test_add_cat_file() {
test_cmp expected actual
'

test_expect_success "ipfs add --chunker rabin-36-512-1024 succeeds" '
ipfs add --chunker rabin-36-512-1024 mountdir/hello.txt >actual
Copy link
Member

Choose a reason for hiding this comment

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

Given that we're not actually checking these results, we should probably just run this command as ipfs add -Q (quiet) and not redirect to actual. We redirect to a file when comparing output with some expected output.

License: MIT
Signed-off-by: Kejie Zhang <601172892@qq.com>
License: MIT
Signed-off-by: Kejie Zhang <601172892@qq.com>
@kjzz
Copy link
Contributor Author

kjzz commented Sep 13, 2018

hey @Stebalien ,Thx a lot for your advice. I have update my test.but maybe something wrong with ci.Can u help me review it?Thx.

@Stebalien
Copy link
Member

Windows CI broke yesterday. I'm rerunning it now.

ipfs add -Q --chunker rabin-36-512-1024 mountdir/hello.txt
'

test_expect_code 127 "ipfs add --chunker rabin-12-512-1024 failed" '
Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, my comment was confusing. This needs to be:

test_expect_success "...." '
    test_expect_code 127 ...
'

(or something like that).

Copy link
Member

Choose a reason for hiding this comment

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

(fixed)

@ghost ghost assigned Stebalien Sep 13, 2018
@ghost ghost added the status/in-progress In progress label Sep 13, 2018
License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@kjzz
Copy link
Contributor Author

kjzz commented Sep 14, 2018

@Stebalien Thx for helping me fix it.

License: MIT
Signed-off-by: Steven Allen <steven@stebalien.com>
@Stebalien
Copy link
Member

Np. Small nits like that are often easier to just fix quickly instead of wasting your time going back and forth.

@kjzz
Copy link
Contributor Author

kjzz commented Sep 14, 2018

Hey @Stebalien , if If you have time,can u give me some advice about issue #4442 and #5432 .And I want to contribute more in ipfs.or if you any issue that i can help you,please ping me.Thx a lot.

@Stebalien Stebalien merged commit 041e771 into ipfs:master Sep 14, 2018
@ghost ghost removed the status/in-progress In progress label Sep 14, 2018
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