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.

Sign up for GitHub

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

Jump to bottom

Add tests for collections to work with ZSTs #30982

Merged
merged 1 commit into from
Jan 21, 2016
Merged

Add tests for collections to work with ZSTs #30982

merged 1 commit into from
Jan 21, 2016

Conversation

KiChjang
Copy link
Member

Fixes #28518.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks! Could you also add one for BTreeMap and VecDeque?

@KiChjang
Copy link
Member Author

VecDeque is in another PR: #30956. I'll add some tests for BTreeMap later.

@KiChjang KiChjang force-pushed the zst-collections-tests branch 6 times, most recently from 395cdff to 845a65f Compare January 18, 2016 17:05
@alexcrichton
Copy link
Member

Ah ok, sounds good to me, thanks!

@bors: r+ 845a65f56033e8aadd21fcf8c6d0d144c54ea8d9

@Gankra
Copy link
Contributor

Gankra commented Jan 18, 2016 

failures:

---- [run-pass] run-pass/zero-sized-btreemap-insert.rs stdout ----

error: test run failed!
status: exit code: 101
command: x86_64-unknown-linux-gnu/test/run-pass/zero-sized-btreemap-insert.stage2-x86_64-unknown-linux-gnu 
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
thread '<main>' panicked at 'assertion failed: `(left == right)` (left: `1`, right: `2`)', /home/travis/build/rust-lang/rust/src/test/run-pass/zero-sized-btreemap-insert.rs:40

------------------------------------------

thread '[run-pass] run-pass/zero-sized-btreemap-insert.rs' panicked at 'explicit panic', /home/travis/build/rust-lang/rust/src/compiletest/runtest.rs:1505

---- [run-pass] run-pass/zero-sized-string-push.rs stdout ----

error: test run failed!
status: exit code: 101
command: x86_64-unknown-linux-gnu/test/run-pass/zero-sized-string-push.stage2-x86_64-unknown-linux-gnu 
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
thread '<main>' panicked at 'assertion failed: `(left == right)` (left: `0`, right: `1`)', /home/travis/build/rust-lang/rust/src/test/run-pass/zero-sized-string-push.rs:28

------------------------------------------

thread '[run-pass] run-pass/zero-sized-string-push.rs' panicked at 'explicit panic', /home/travis/build/rust-lang/rust/src/compiletest/runtest.rs:1505


failures:
    [run-pass] run-pass/zero-sized-btreemap-insert.rs
    [run-pass] run-pass/zero-sized-string-push.rs

test result: �[31mFAILED�(B�[0m. 2351 passed; 2 failed; 2 ignored; 0 measured

Gankra
Gankra reviewed Jan 18, 2016
View reviewed changes
src/test/run-pass/zero-sized-string-push.rs
tester.clear();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Strings store bytes, there's no ZST stuff to test...?

Copy link
Contributor

Choose a reason for hiding this comment

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

Regardless the test is incorrect, the length of the string should never increase.

@KiChjang
Copy link
Member Author

Hopefully I've addressed the comments with the latest commit.

@Gankra
Copy link
Contributor

Gankra commented Jan 18, 2016

@bors r+

Thanks for the quick response!

@bors
Copy link
Contributor

bors commented Jan 18, 2016

📌 Commit 0433967 has been approved by Gankro

@Manishearth Manishearth mentioned this pull request Jan 18, 2016
Merged
@KiChjang
Copy link
Member Author

Travis CI was failing the previous commit; I've just updated it again.

@KiChjang KiChjang force-pushed the zst-collections-tests branch 2 times, most recently from eff436f to 42d7b9d Compare January 18, 2016 22:50
bors added a commit that referenced this pull request Jan 18, 2016
@bors
Auto merge of #31006 - Manishearth:rollup, r=Manishearth
7a7307e 
- Successful merges: #30981, #30982, #30986, #30987, #30988, #30990, #30998
- Failed merges:
@KiChjang
Copy link
Member Author

Did this actually get merged, or do we still need to r+ this?

@alexcrichton
Copy link
Member

@bors: r=Gankro 42d7b9dbd7b4fa667be813a426a15175eb7447e8

@bors
Copy link
Contributor

bors commented Jan 20, 2016

⌛ Testing commit 42d7b9d with merge cbeae86...

bluss
bluss reviewed Jan 20, 2016
View reviewed changes
src/test/run-pass/zero-sized-btreemap-insert.rs
}
assert_eq!(tester.len(), if len == 0 { 0 } else { 1 });
assert_eq!(tester.iter().count(), if len == 0 { 0 } else { 1 });
tester.clear();
Copy link
Member

Choose a reason for hiding this comment

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

Why do these assertions pass? The Zst is made to not be equal to itself (by PartialOrd, PartialEq), so n insertions should make a map with n elements?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! I misread the test... that's weird.

Copy link
Member

Choose a reason for hiding this comment

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

It's not zst specific, here's a non-zst https://play.rust-lang.org/?gist=0b20ed7ae5c5fb666bc3&version=nightly

and zst: https://play.rust-lang.org/?gist=5e63c60d587015a4ef2f&version=nightly

And wonky behavior is totally permissible, because the key type implements Ord but does not really have a total order (it's a broken trait impl).

@bluss
Copy link
Member

bluss commented Jan 20, 2016

@KiChjang I think the btreemap test needs to be fixed. The whole broken ordering thing kind of comes from me, I mentioned it as a possibility, but I didn't think of that it breaks the Ord guarantees.

The only way to have a total order over Zst is to have them all equal.

  • Dispatching comparison on the address will break if rust changes how it merges / not merges zst values. It also breaks the not explicitly spelled out invariant that the same value should not change how it compares if you move the value(??).

Edit: The btreemap test needs to be fixed.

@KiChjang
Copy link
Member Author

Okay, so this means instead of impl PartialOrd being Ordering::Less, it should be Ordering::Equal instead?

@bluss
Copy link
Member

bluss commented Jan 20, 2016

Just deriving all comparisons should work. Remove the case loop, it's redundant (I think?)

@KiChjang
Copy link
Member Author

Done... but is there a way to verify the tests work locally before I commit?

@bluss
Copy link
Member

bluss commented Jan 20, 2016

Yes. make check-stage1-rpass TESTNAME=zero-sized for example (or stage2, also works)

bluss
bluss reviewed Jan 20, 2016
View reviewed changes
src/test/run-pass/zero-sized-string-push.rs
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use std::string::String;
Copy link
Member

Choose a reason for hiding this comment

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

This file could be removed? There is no ZSTs here.

@KiChjang KiChjang force-pushed the zst-collections-tests branch 2 times, most recently from 004b559 to aca4e6a Compare January 20, 2016 15:51
@bluss
Copy link
Member

bluss commented Jan 20, 2016

Did it work locally?

@KiChjang
Copy link
Member Author

Yes, it did.

@bluss
Copy link
Member

bluss commented Jan 20, 2016

Great, let's go again!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 20, 2016

📌 Commit aca4e6a has been approved by bluss

@bors
Copy link
Contributor

bors commented Jan 20, 2016

⌛ Testing commit aca4e6a with merge 7dce32e...

bors added a commit that referenced this pull request Jan 20, 2016
@bors
Auto merge of #30982 - KiChjang:zst-collections-tests, r=bluss
7dce32e 
Fixes #28518.
@bors
Copy link
Contributor

bors commented Jan 21, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors bors merged commit aca4e6a into rust-lang:master Jan 21, 2016
@KiChjang KiChjang deleted the zst-collections-tests branch January 21, 2016 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@brson brson

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@KiChjang @rust-highfive @alexcrichton @Gankra @bors @bluss @brson