Skip to content

word-search: Update tests to version 1.1.0(#1012) #1053

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.

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

Merged
merged 10 commits into from
Oct 27, 2017

Conversation

josix
Copy link
Contributor

@josix josix commented Oct 26, 2017

resolve issue #1012

Copy link
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.

@wilson8507 I've left some comments, please address them.

@@ -36,48 +137,179 @@ def test_vertical_words_top_to_bottom(self):
self.example.search('ecmascript'),
(Point(9, 0), Point(9, 9))
)
self.assertEqual(
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of redundant and misplaced assertions. Please check that each assertion actually belongs under its test method (Example: checks for "clojure" in self.example should only appear in tests for horizontal words).

josix added 3 commits October 27, 2017 01:33
- Delete assertions that appearing in multiple test methods
- Make sure each test case appearing in correct test methods
@josix
Copy link
Contributor Author

josix commented Oct 26, 2017

@cmccandless I've fixed it, please check if there is any problem. Thanks!

@@ -2,9 +2,112 @@

from word_search import WordSearch, Point

# Tests adapted from `problem-specifications//canonical-data.json` @ v1.1.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor formatting point, but can you move the version comment down a line (ie. two blank lines before, one after).

Copy link
Contributor

@N-Parsons N-Parsons left a comment

Choose a reason for hiding this comment

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

I've left a comment regarding a minor formatting issue. Once you've resolved it, I'm happy merge this.

Thanks for the hard work, @wilson8507!

@N-Parsons N-Parsons self-assigned this Oct 26, 2017
@josix
Copy link
Contributor Author

josix commented Oct 27, 2017

@N-Parsons I've resolved it. Thank you for pointing out my carelessness!

@N-Parsons
Copy link
Contributor

Perfect! Thanks, @wilson8507! I'm a bit picky about formatting and consistency 😛

@N-Parsons N-Parsons merged commit f0dfd60 into exercism:master Oct 27, 2017
josix added a commit to josix/python that referenced this pull request Oct 28, 2017
)

* word-search: Update tests to version 1.1.0(exercism#1012)

* Remove redundant and misplaced assertions
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.

4 participants