Skip to content

Add test template for atbash cipher#2123

Merged
yawpitch merged 10 commits into
exercism:masterfrom
tqa236:atbash-cipher-add-test-template
Nov 14, 2019
Merged

Add test template for atbash cipher#2123
yawpitch merged 10 commits into
exercism:masterfrom
tqa236:atbash-cipher-add-test-template

Conversation

@tqa236
Copy link
Copy Markdown
Contributor

@tqa236 tqa236 commented Nov 12, 2019 •
edited by cmccandless
Loading

Resolve #1928.

There's a track specific test that is not included yet. I also don't see that this test is quite interesting or relevant. What do you think about it?

@tqa236 tqa236 requested a review from a team as a code owner November 12, 2019 20:40
cmccandless
cmccandless approved these changes Nov 12, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@cmccandless cmccandless left a comment

Choose a reason for hiding this comment

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

I'm OK with losing the round-trip test. @yawpitch?

@yawpitch
Copy link
Copy Markdown
Contributor

yawpitch commented Nov 13, 2019

I'm okay with losing the roundtrip as well.

Tangent, but why are we using assertMultiLineEqual when none of the strings are multiple lines? Also since we've dropped support for 2 and 3.4 is our minimum 3 we could just switch back to assertEqual, which uses assertMultiLineEqual under the hood for strings anyway.

@tqa236
Copy link
Copy Markdown
Contributor Author

tqa236 commented Nov 13, 2019

I don't know the difference between these two so I use the one in the old test file. Should I switch to assertEqual now?

@yawpitch
Copy link
Copy Markdown
Contributor

yawpitch commented Nov 13, 2019

I'd say so ... @cmccandless we got anything running < 3.4 on CI?

@cmccandless
Copy link
Copy Markdown
Contributor

cmccandless commented Nov 13, 2019

we got anything running < 3.4 on CI?

Not anymore, no.

@tqa236
Copy link
Copy Markdown
Contributor Author

tqa236 commented Nov 14, 2019

Is this PR ready to be merged or should I modify anything?

yawpitch
yawpitch approved these changes Nov 14, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@yawpitch yawpitch left a comment

Choose a reason for hiding this comment

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

Yeah, since we're not using anything old enough to break on the assertEqual change. I'll merge soon as the tests finish after merging in the latest.

@yawpitch yawpitch merged commit fd006da into exercism:master Nov 14, 2019
@tqa236 tqa236 deleted the atbash-cipher-add-test-template branch November 14, 2019 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@yawpitch yawpitch yawpitch approved these changes

@cmccandless cmccandless cmccandless approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

atbash-cipher: add test template

3 participants

@tqa236 @yawpitch @cmccandless