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

Adding tests for replaceSubrange overloads for range types #2774

Merged
merged 1 commit into from
Jun 7, 2016
Merged

Adding tests for replaceSubrange overloads for range types #2774

merged 1 commit into from
Jun 7, 2016

Conversation

austinzheng
Copy link
Contributor

@austinzheng austinzheng commented May 30, 2016 •
edited
Loading

Tests for RangeReplaceableCollection's new replaceSubrange() overloads, for use with the new ranges.

EDIT: Tests all run and pass successfully, this PR can be merged at any time.

@gribozavr

Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

@gribozavr
Copy link
Contributor

@swift-ci Please test

gribozavr
gribozavr reviewed May 30, 2016
View reviewed changes
stdlib/private/StdlibCollectionUnittest/CheckRangeReplaceableCollectionType.swift
@@ -44,6 +51,78 @@ internal enum RangeSelection {
return start..<end
}
}

internal func countableRange<C : Collection>(in c: C) -> CountableRange<C.Index> {
switch self {
Copy link
Contributor

Choose a reason for hiding this comment

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

return CountableRange(range(in: c)) ?

@gribozavr
Copy link
Contributor

We already generate MinimalRandomAccessCollectionWithStrideableIndex. Let's move the tests for overloads on countable ranges into validation-test/stdlib/RangeReplaceable.swift.gyb and run them on MinimalRandomAccessCollectionWithStrideableIndex. Since these overloads are not customizable and the implementation is very simple, I don't feel bad about not running the tests on all collections.

gribozavr
gribozavr reviewed May 30, 2016
View reviewed changes
stdlib/private/StdlibCollectionUnittest/CheckRangeReplaceableCollectionType.swift
var c = makeWrappedCollection(test.collection)
let rangeToReplace = test.rangeSelection.closedRange(in: c)
let newElements =
MinimalCollection(elements: test.newElements.map(wrapValue))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be using makeWrappedCollection.

@austinzheng
Copy link
Contributor Author

Thank you for the feedback! I will have an updated PR for you tonight.

All tests were passing for me before I pushed, so I will investigate to see why.

@austinzheng austinzheng force-pushed the validation-tests branch 4 times, most recently from 5d9d06a to 32bbcaa Compare June 3, 2016 06:12
@austinzheng
Copy link
Contributor Author

@gribozavr I've rewritten the tests and everything passes now. I'll do String next, once this clears.

@gribozavr
Copy link
Contributor

Thanks Austin! I didn't look closely at the patch, but let's trigger the CI:

@swift-ci Please test

@austinzheng
Copy link
Contributor Author

If there are no problems, you can push it whenever CI passes. The tests passed locally on my box.

@austinzheng
Copy link
Contributor Author

austinzheng commented Jun 3, 2016 •
edited
Loading

I don't think the test failures are related to this commit.

On OS X, this test (which involves a file that seems to have been touched by a recent commit) fails:

/Users/buildnode/jenkins/workspace/swift-PR-osx/swift/validation-test/Reflection/inherits_NSObject.swift:42:14: error: expected string not found in input
// CHECK-32: (class_instance size=4 alignment=8 stride=8 num_extra_inhabitants=0)
             ^
<stdin>:7:1: note: scanning from here
(class_instance size=4 alignment=16 stride=16 num_extra_inhabitants=0)

On Linux, the error looks like this:

[TestExternCSymbols.py FAILED]

@austinzheng austinzheng force-pushed the validation-tests branch 2 times, most recently from d674eac to 4a94899 Compare June 4, 2016 16:20
@austinzheng
Copy link
Contributor Author

Just wanted to check in on this PR @gribozavr. If you're busy I can start working on the String tests concurrently.

@gribozavr
Copy link
Contributor

@austinzheng Sorry for the silence! I will try to get a review soon. Please start as many concurrent PRs as you are comfortable with.

@austinzheng
Copy link
Contributor Author

austinzheng commented Jun 7, 2016 •
edited
Loading

I have a branch with similar tests to these, but for removeSubrange (https://github.com/austinzheng/swift/tree/az-removesubrange-tests).

I'll make a PR after this one is merged, because that branch depends on changes in this one. I'll also get started on the String tests.

austinzheng
austinzheng reviewed Jun 7, 2016
View reviewed changes
stdlib/private/StdlibCollectionUnittest/CheckRangeReplaceableCollectionType.swift
@@ -12,14 +12,14 @@

import StdlibUnittest

internal enum RangeSelection {
public enum RangeSelection {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to end up using this enum from at least two more test files. Would it be a good idea to move it into a new 'Utilities' style file?

@austinzheng austinzheng mentioned this pull request Jun 7, 2016
Merged
1 task
@gribozavr
Copy link
Contributor

Would it be a good idea to move it into a new 'Utilities' style file?

No objections, but I'd prefer to call it RangeSelection.swift if it is just that type.

@austinzheng austinzheng force-pushed the validation-tests branch 2 times, most recently from a8fbf7f to 85e0695 Compare June 7, 2016 16:11
@austinzheng
Copy link
Contributor Author

I moved the RangeSelection enum into RangeSelection.swift. Everything seems to be working.

@moiseev
Copy link
Contributor

moiseev commented Jun 7, 2016

@swift-ci Please test

@moiseev
Copy link
Contributor

moiseev commented Jun 7, 2016

@austinzheng PR looks good to me. Is it ready to be merged? Please modify the description if so ;-) Thanks.

@austinzheng
Copy link
Contributor Author

@moiseev Done, thanks so much! It is indeed ready to be merged.

@moiseev moiseev merged commit fa6c028 into swiftlang:master Jun 7, 2016
@austinzheng austinzheng deleted the validation-tests branch June 8, 2016 02:42
MaxDesiatov pushed a commit that referenced this pull request Apr 19, 2021
@pull
Merge pull request #2774 from swiftwasm/main
4843d91 
[pull] swiftwasm from main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@austinzheng @gribozavr @moiseev