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.

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

triangle : add test template #2016

Merged
merged 5 commits into from
Oct 3, 2019

Conversation

mpatibandla
Copy link
Contributor

No description provided.

@mpatibandla mpatibandla requested a review from a team as a code owner October 3, 2019 06:35
def test_{{ case["description"] | to_snake }}(self):
input = {{ case["input"]["sides"] }}
expected = {{ case["expected"] }}
self.assertEqual({{ case["property"] }}(input), expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend putting everything inside the assertEqual call instead of using the input and expected variables here. You can spread things out like so for template readability, and the generator will put everything on a single line in generation:

self.assertEqual(
    {{ case["property"] }}({{ case["input"]["sides"] }}),
    {{ case["expected"] }}
)

@mpatibandla
Copy link
Contributor Author

Made changes as requested. Thanks !

class {{ case["description"] | camel_case }}Test(unittest.TestCase):
{% for case in case["cases"] %}
def test_{{ case["description"] | to_snake }}(self):
self.assertEqual({{ case["property"] }}({{ case["input"]["sides"] }}), {{ case["expected"] }})
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, smaller diff let me see this one. From our conventions:

We use assertIs(actual, True) and assertIs(actual, False) rather than assertTrue(actual) or assertFalse(actual) (#419).

Suggested change
self.assertEqual({{ case["property"] }}({{ case["input"]["sides"] }}), {{ case["expected"] }})
self.assertIs({{ case["property"] }}({{ case["input"]["sides"] }}), {{ case["expected"] }})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to assertIs as per conventions. Thanks.

@cmccandless cmccandless merged commit 7aac5be into exercism:master Oct 3, 2019
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.

2 participants