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

Do not allow dots on test names #18

Merged
merged 1 commit into from
Mar 20, 2014
Merged

Do not allow dots on test names #18

merged 1 commit into from
Mar 20, 2014

Conversation

elyezer
Copy link
Contributor

@elyezer elyezer commented Mar 17, 2014

Dots is not allowed on function/methods names, so the names generated for ddt should not allow dots on the generated name.

This pull request ensure that there are no dots on ddt generated function/method names.

@omaciel
Copy link

omaciel commented Mar 17, 2014

Niiice, thanks @elyezer

fwiw here's what we were seeing:

<testcase
classname="tests.cli.test_subnet.TestSubnet.test_update_success_ddt_{'dns-primary':
'192.168" name="100" time="4.841"/>

@elyezer
Copy link
Contributor Author

elyezer commented Mar 17, 2014

Also this will improve the XUnit nose output by correctly handling the test name as @omaciel commented.

@txels
Copy link
Collaborator

txels commented Mar 18, 2014

There was an issue raised for a similar case: #4

I'd like to have a more general approach to processing generated function names (and maybe as an opt-in, but it's OK if it's not I guess) so that it covers a wide range of cases.

In general I haven't found it to be a problem when using DDT that generated method names are not valid python identifiers, as they do not have to go through a parser. To some extent one could argue that it guarantees no clash with properly declared methods. It seemed to be a problem only with test result reporters down the line. Can you give me some background about why this was a problem in your case?

@omaciel
Copy link

omaciel commented Mar 19, 2014

@txels in our case we have several data-driven tests that use random data that occasionally have a dot (".") in it, such as data for subnet, IP or even email addresses (eg. [email protected]). Once nose sees the test names and reports them back, the dot in the data ends up causing the test name to be incorrect.

Here's an example for what nose reports back via Jenkins:

screen shot 2014-03-18 at 9 50 28 pm

The tests.cli.test_user.User.test_negative_create_user_4_Abc test should be included as part of the tests.cli.test_user.User module but since the data being passed is {'email': 'abc.example.com'} the dot causes the module to be named tests.cli.test_user.User.test_negative_create_user_4_Abc, the name becoming com.

@elyezer
Copy link
Contributor Author

elyezer commented Mar 19, 2014

@txels @omaciel have illustrated our problem with having dots. This fixed that and make sure that the methods names follows the python naming conventions.

name, value.encode('ascii', 'backslashreplace')
)
return test_name.replace('.', '_')
Copy link
Collaborator

Choose a reason for hiding this comment

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

So I understand your purpose is to generate a valid python identifier as a test name so that reporting tools that process this test name (like the Junit reporter) don't choke.

This PR doesn't fully achieve that purpose, as it only caters for a very specific case.

What about something like this (source http://stackoverflow.com/a/3305731/166761):

return re.sub('\W|^(?=\d)','_', test_name)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will update with that. Thanks
On Mar 19, 2014 6:52 PM, "Carles Barrobés i Meix" [email protected]
wrote:

In ddt.py:

         name, value.encode('ascii', 'backslashreplace')
     )
  • return test_name.replace('.', '_')

So I understand your purpose is to generate a valid python identifier as a
test name. This doesn't quite achieve it, it only caters for a very
specific case.

What about something like this (source
http://stackoverflow.com/a/3305731/166761):

test_name = re.sub('\W|^(?=\d)','_', test_name)


Reply to this email directly or view it on GitHubhttps://github.com//pull/18/files#r10773525
.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool great I could certainly go for that. Can you update test accordingly?

@elyezer
Copy link
Contributor Author

elyezer commented Mar 20, 2014

@txels I have updated the code and tests. Also I have to update the test_ddt_data_unicode test because the re substitution. Also it is generating some "weird" test names.

Please let me know if I need to improve or change something.

@txels
Copy link
Collaborator

txels commented Mar 20, 2014

This looks good to merge. I may take a look at unicode, maybe we need a different approach to test naming in that case. Thanks!

txels added a commit that referenced this pull request Mar 20, 2014
Do not allow dots on test names
@txels txels merged commit 1b25f2a into datadriventests:master Mar 20, 2014
@txels
Copy link
Collaborator

txels commented Mar 20, 2014

@elyezer I have one concern left before I release this version, which is that different data values may now generate the same test names, as the name conversion is lossy. E.g. two data values "hello_mum" and "hello.mum" will essentially generate the same test name, and setattr will override it.

I need to rethink how to make sure names are unique. Any ideas and contributions are welcome of course.

@omaciel
Copy link

omaciel commented Mar 20, 2014

How about generating a unique ID akin the likes of uuid? For most part a "regular" use case would not care about the test name including the data attributes.

@elyezer
Copy link
Contributor Author

elyezer commented Mar 20, 2014

That is a problem that I have not thought, good catch.

I have some suggestions that we can evaluate, like creating a base64 string based on the value, the uuid suggested by @omaciel. But the problem with that could be identifying the data passed, we could have a unique suffix also.

I will think better and if I came with something I will contribute for sure.

Thanks for the merge.

@txels
Copy link
Collaborator

txels commented Mar 21, 2014

I have thought about this and the simplest thing is to add a sequential number to each multiplied test case, starting with 1:

@data('hell.no', 'hell_no')
def test_it(self):

Would generate:

test_it_1_hell_no
test_it_2_hell_no
...

I'll work on this fix now.

@elyezer
Copy link
Contributor Author

elyezer commented Mar 21, 2014

I was about to suggest using the parameter index staring with 0. As the data receive *args so the index could be used. But starting with 1 will do the trick too.

Please let me know if I can help with something.

@txels
Copy link
Collaborator

txels commented Mar 21, 2014

Here it is: 5c00b6e
Released as v0.8.0

@omaciel
Copy link

omaciel commented Mar 21, 2014

Thanks @txels a million!!!

@elyezer
Copy link
Contributor Author

elyezer commented Mar 21, 2014

Thanks @txels and congratulations for the really useful project.

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