Add docstrings to tests #27

LilyFirefly · 2014-10-07T21:41:57Z

No description provided.

LilyFirefly · 2014-10-07T21:43:09Z

conman/nav_tree/tests/test_utils.py

I would have expected ImportError. Is there a particular reason ValueError is more appropriate?

It's a peculiarity of implementation, I suppose. Perhaps we should not have this limitation (though I cannot see when we would need a top-level package here.)

Hmm. Thinking about it a bit more, maybe ValueError is appropriate - the function is designed to work only with dotted paths. I think it should have a custom error message though.

ValueError is appropriate - the function is designed to work only with dotted paths

Indeed.

I think it should have a custom error message though.

👍

coveralls · 2014-10-07T21:49:25Z

Coverage remained the same when pulling 4a3b2e5 on Ian-Foote:test-docstrings into accdec8 on meshy:master.

meshy · 2014-10-07T21:51:22Z

@ian-foote thank-you for the help :)

coveralls · 2014-10-07T22:00:48Z

Coverage remained the same when pulling d5572f6 on Ian-Foote:test-docstrings into accdec8 on meshy:master.

LilyFirefly · 2014-10-07T22:03:59Z

conman/nav_tree/tests/test_views.py

These tests seem complicated - they were difficult to condense into docstring summaries. Perhaps they can be simplified?

They do. Not very DRY either. Perhaps if they were made dry, then it would look easier to have one assert per test here.

* Split tests for permanent/temporary redirects from test for the redirect's target url * Simplify integration test to focus on integration * Remove essentially duplicate integration test * Add docstrings

LilyFirefly · 2014-10-07T22:43:05Z

@meshy Review?

LilyFirefly · 2014-10-07T22:44:03Z

conman/redirects/tests/test_views.py

I think this refactor is an improvement, but if you disagree or think it shouldn't be in this pull-request I can easily remove it.

I think this refactor is an improvement

So do I, thanks :)

coveralls · 2014-10-07T22:47:16Z

Coverage remained the same when pulling 48c2004 on Ian-Foote:test-docstrings into accdec8 on meshy:master.

meshy · 2014-10-07T22:52:34Z

👍 Thank-you very much! This may be a hard standard to keep up, but I hope we can do it!

Add docstrings to tests

Ian Foote added 3 commits October 7, 2014 22:23

Add docstrings to nav_tree.tests.test_models

8d69656

Add docstring to nav_tree.tests.test_urls

ba0b4eb

Add docstrings to nav_tree.tests.test_utils

4a3b2e5

LilyFirefly reviewed Oct 7, 2014
View reviewed changes

Add docstrings to nav_tree.tests.test_views

d5572f6

LilyFirefly reviewed Oct 7, 2014
View reviewed changes

Ian Foote added 6 commits October 7, 2014 23:07

Add docstrings to pages.tests.test_handlers

c1931b3

Add docstring to pages.tests.test_models

e41a07b

Add docstrings to pages.tests.test_views

0ed6ed6

Add docstrings to redirects.tests.test_handlers

db26c88

Add docstrings for redirects.tests.test_models

4046cbf

Refactor redirects.tests.test_views

48c2004

* Split tests for permanent/temporary redirects from test for the redirect's target url * Simplify integration test to focus on integration * Remove essentially duplicate integration test * Add docstrings

LilyFirefly reviewed Oct 7, 2014
View reviewed changes

This was referenced Oct 7, 2014

Improve error message on nav_tree.utils.import_from_dotted_path #28

Closed

DRY out tests.routes.test_views #29

Open

meshy added a commit that referenced this pull request Oct 7, 2014

Merge pull request #27 from Ian-Foote/test-docstrings

2d71666

Add docstrings to tests

meshy merged commit 2d71666 into meshy:master Oct 7, 2014

LilyFirefly deleted the test-docstrings branch October 7, 2014 22:54

LilyFirefly mentioned this pull request Oct 7, 2014

Docstring all tests #11

Closed

LilyFirefly self-assigned this Oct 7, 2014

Add docstrings to tests #27

Add docstrings to tests #27

Uh oh!

Conversation

LilyFirefly commented Oct 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 7, 2014

Uh oh!

meshy commented Oct 7, 2014

Uh oh!

coveralls commented Oct 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LilyFirefly commented Oct 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Oct 7, 2014

Uh oh!

meshy commented Oct 7, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants