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

Add option to split by class/module + cleanup #4

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Jeff-Meadows
Copy link

No description provided.

Add tests for this and a test to make sure all tests are picked up.
Fix a few style issues.
Remove broken test envs - hash_ring isn't py3 compatible, so neither is this, and conf.py is missing so docs don't work either.
Add Travis CI integration.
@winhamwr
Copy link
Contributor

Hi Jeff,

Thanks for this! Lots of good cleanup here and I like the idea of providing control of where things can be split. I'll plan on giving this a good look on Tuesday.

My initial impressions:

  • The default for modules should be to split
  • The default for classes should be to not split
  • We'll need to document this
  • Why the dunder variable name? Do you see that with other nose plugin things?

@Jeff-Meadows
Copy link
Author

Hi Wes,

Thanks for taking a look.

You're right that this feature would need documentation. As for the dunder, it was a misreading of the way the built in multiprocess nose plugin did things. I wanted to keep it consistent. I'll submit another commit where it's a single leading and trailing underscore. 😄

@winhamwr
Copy link
Contributor

winhamwr commented Sep 3, 2014

Hi Jeff,

Sorry for being so slow to review this. I wanted to get to it today. Tomorrow is another day :)

-Wes

@@ -103,9 +104,33 @@ def _options_are_valid(self):

return True

def getCannotSplit(self, testObject):
Copy link
Contributor

Choose a reason for hiding this comment

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

So it looks like what we're actually returning here is returning either the class or the module if we can't split it, right?

I think we could simplify this code and reduce the duplication inside validateName by making this method instead always return a string representing the lowest level at which this method/class/module can be split. So for things that can be split, we'd return '%s.%s' % (module, call). For a module that can't be split, we'd return '%s' % module. For a class that can't be split, '%s' % klass

Maybe this could be called getLowestSplitLevelHash? It should also probably include a quick docstring to this effect.

Copy link
Author

Choose a reason for hiding this comment

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

Great suggestion and I even uncovered some bugs/weird behaviors while refactoring.

This code will split at the class level even if the module specifies no splitting, but that's fixed in the next commit.

@winhamwr
Copy link
Contributor

winhamwr commented Sep 4, 2014

Hi Jeff,

Thanks again for this pull request. Controlling the split behavior is super useful.

Would you mind adding yourself to AUTHORS and writing up a quick CHANGELOG entry for 0.2.0 announcing your awesome new feature?

-Wes

Update CHANGELOG and version number.
Refactor how the splitting works and add a bit more documentation.
@Jeff-Meadows
Copy link
Author

/poke =D

@Jeff-Meadows
Copy link
Author

Hey Wes,

I'd really love to see this merged. Anything you need from me to make it happen?

@kylegibson
Copy link
Member

@Jeff-Meadows Wes is actually on vacation until about the end of the month, so it will be a few weeks at least until he can look at this again.

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