Skip to content

crypto-square: tests are now generated#739

Closed
ajwann wants to merge 1 commit intoexercism:masterfrom
ajwann:crypto-square-tests-should-be-generated
Closed

crypto-square: tests are now generated#739
ajwann wants to merge 1 commit intoexercism:masterfrom
ajwann:crypto-square-tests-should-be-generated

Conversation

@ajwann
Copy link
Copy Markdown
Contributor

@ajwann ajwann commented Oct 15, 2017

Why?

All tests should be generated as per #396

What?

This PR contains a generator for the crypto-square exercise.

How Has This Been Tested?

bin/generate crypto-square
rake test:crypto-square

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Breaking change (fix or enhancement that would cause existing functionality to change)
  • Gardening (code and/or documentation cleanup)

References and Closures

#396

Checklist:

  • I have read the CONTRIBUTING document.
  • My change relies on a pending issue/pull request
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

class CryptoSquareCase < Generator::ExerciseCase

# TODO: remove guard clause when #encoded method
# is added to crypto_square.rb
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.

So the canonical data is a bit off for this exercise. There are several tests in the canonical data that are expecting a method named encode or possibly encoded to be defined on the Crypto class. However, this method is missing from crypto_square.rb

end
end

# TODO: remove when canonical data is fixed
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.

The canonical data for one test also has an extra space, which causes the assertion to fail.

end

def plaintext_segments
return [] if normalize_plaintext == ''
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.

The canonical data had a test which asserts that an empty array passed into Crypto.plaintext_segments will return an empty array. However, the current Crypto class just blows up when it tries to call each_slice with a size of 0.

end

def ciphertext
return '' if normalize_plaintext == ''
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.

The canonical data also had a test which asserts that an empty array passed into Crypto.ciphertext will return an empty string. However, the current Crypto class still just blows up when it tries to call each_slice with a size of 0.

crypto = Crypto.new('123456789')
assert_equal 3, crypto.size
crypto = Crypto.new('Ab Cd')
assert_equal ["ab", "cd"], crypto.plaintext_segments
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.

So these tests seemed to have assertion that differed from what the canonical data wanted.
I went with the canonical data's choice of assertions, because I figured it was more up-to-date.

@nholden
Copy link
Copy Markdown

nholden commented Oct 16, 2017

Hi, @ajwann! It looks like this PR is using an older version of the canonical data. I submitted #738, which uses the latest version of the canonical data. Would you mind taking a look and letting me know what you think?

@Insti
Copy link
Copy Markdown
Contributor

Insti commented Oct 17, 2017

It looks like this is a case of "great minds think alike" and we have 2 PRs to generate tests for the same problem.

#738 Looks like a currently more complete and functional solution so I'll close this one.

Thanks @ajwann for your work on this. ❤️

@Insti Insti closed this Oct 17, 2017
@ajwann
Copy link
Copy Markdown
Contributor Author

ajwann commented Oct 26, 2017

@nholden I should have though to pull from master in my x-common repo! Oh well, nice work with the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants