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

Group tuple tests into @testsets #22606

Merged
merged 3 commits into from
Jun 30, 2017
Merged

Group tuple tests into @testsets #22606

merged 3 commits into from
Jun 30, 2017

Conversation

HarrisonGrodin
Copy link
Contributor

Solves #18807.

tkelman
tkelman reviewed Jun 29, 2017
View reviewed changes
test/tuple.jl
@test ntuple(identity, Val{n}) == ntuple(identity, n)
end

# Tuple type ninitialized
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like you deleted this set without a replacement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkelman Fixed. My mistake.

@ararslan ararslan added domain:collections Data structures holding multiple items, e.g. sets test This change adds or pertains to unit tests labels Jun 29, 2017
@kshyatt
Copy link
Contributor

kshyatt commented Jun 29, 2017

Welcome to base Julia, @HarrisonGrodin! Thanks for this PR - looks like tests are failing because of some sort of syntax issue. Can you run make test-tuple locally and see if you can fix it? Feel free to PM me on Discourse if you get stuck.

@HarrisonGrodin
Copy link
Contributor Author

@kshyatt The issue has been fixed; sorry about that.

For some strange reason, I wasn't able to run make without errors on master, but it now seems to work after a full recompile. 🙂

tkelman
tkelman reviewed Jun 29, 2017
View reviewed changes
test/tuple.jl
sort(collect(p)) != collect(0:7) && error("$p is not a permutation of 0:7")
new(p)
end
BitPerm_19352(xs::Vararg{Any,8}) = BitPerm(map(UInt8, xs))
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit of a refactoring of the constructor, hopefully it would still reproduce the original issue in this form?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tkelman Yep! I tested it in v0.5, where the bug still existed, and it still produces the same behavior. Just a bit more concise.

tkelman
tkelman approved these changes Jun 29, 2017
View reviewed changes
Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

looks like everything's still here despite a bit of reordering, so lgtm - should definitely be squashed on merging since intermediate commits failed

@ararslan ararslan merged commit 32422d4 into JuliaLang:master Jun 30, 2017
@fredrikekre fredrikekre mentioned this pull request Jul 24, 2017
Closed
@fredrikekre fredrikekre mentioned this pull request Sep 30, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman approved these changes

Assignees
No one assigned
Labels
domain:collections Data structures holding multiple items, e.g. sets test This change adds or pertains to unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@HarrisonGrodin @kshyatt @tkelman @ararslan