Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Additional String.Join tests#2839

Merged
stephentoub merged 4 commits into
dotnet:masterfrom
bbowyersmyth:StringJoinTests
Aug 18, 2015
Merged

Additional String.Join tests#2839
stephentoub merged 4 commits into
dotnet:masterfrom
bbowyersmyth:StringJoinTests

Conversation

@bbowyersmyth
Copy link
Copy Markdown
Contributor

Added more test cases for:

String.Join(String, String[])
String.Join(String, String[], int, int)
String.Join(String, Object[])
String.Join(String, IEnumerable<String>)
String.Join(String, IEnumerable<T>)

@dnfclas
Copy link
Copy Markdown

dnfclas commented Aug 17, 2015

Hi @bbowyersmyth, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of Assert.True you can use Assert.Equal here and below. It prints a much better message when the test case fails.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Updated this file to be consistent.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for changing all of these to be Equal instead of True. However, the first argument to Equal is meant to be the expected value, the second the actual value, so most (all?) of these need to have their arguments swapped. Otherwise, if the assert fails, the error message that gets output ends up being confusing. Could you swap them where appropriate as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Swapped the arguments + updated some of the existing ones that were in the wrong order.

@stephentoub
Copy link
Copy Markdown
Member

Thanks for the updates, @bbowyersmyth. I know you've submitted changes to the corefx repo in the past, but the bot is saying that you still need to sign the CLA. Maybe your previous changes were small enough that this wasn't needed? Or maybe you've changed your account in the interim or something?

@bbowyersmyth
Copy link
Copy Markdown
Contributor Author

@stephentoub The CLA was signed on the 1st of Jan (probably your 31st Dec). I can forward you the confirmation email if that helps match things up?

@stephentoub
Copy link
Copy Markdown
Member

@joshfree, can you help sort out why the CLA bot is marking @bbowyersmyth's PRs with "CLA required"?

@bbowyersmyth
Copy link
Copy Markdown
Contributor Author

@stephentoub @joshfree It does say it is for project "ASP.NET MVC". Is another CLA required for corefx or is this for all MS Open Tech?

@stephentoub
Copy link
Copy Markdown
Member

@bbowyersmyth, ah, thanks, I believe that's a different CLA. You need to complete the one specifically mentioned in the comment from the bot earlier. Thanks!

@bbowyersmyth
Copy link
Copy Markdown
Contributor Author

@stephentoub Ok will do

@joshfree
Copy link
Copy Markdown
Member

@stephentoub @bbowyersmyth yup - everyone signs the specific CLA for dotnetfoundation; this will cover you for /dotnet/corefx, /dotnet/coreclr, /dotnet/wcf... /Microsoft/* may use different CLAs.

@dnfclas
Copy link
Copy Markdown

dnfclas commented Aug 17, 2015

@bbowyersmyth, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@stephentoub
Copy link
Copy Markdown
Member

Thanks, @bbowyersmyth. There's probably a more consolidated way to write some of these new tests (e.g. with theories), but the style you used matches what's already there, so it's fine to stick with for now. LGTM.

@bbowyersmyth
Copy link
Copy Markdown
Contributor Author

Yeah I tried to match the style that was there.

stephentoub added a commit that referenced this pull request Aug 18, 2015
@stephentoub stephentoub merged commit 3e11f82 into dotnet:master Aug 18, 2015
@bbowyersmyth bbowyersmyth deleted the StringJoinTests branch August 19, 2015 02:49
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants