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

tests, CI and pep8 #45

Open
markroxor opened this issue Sep 25, 2018 · 6 comments
Open

tests, CI and pep8 #45

markroxor opened this issue Sep 25, 2018 · 6 comments

Comments

@markroxor
Copy link
Collaborator

@brohrer I suggest that so as to avoid any breaking changes we make through out developing Becca, we should write tests for each module and use some sort of continuous integration like Travis CI which requires a .travis.yml file something like this.

We can use tox to automate our tests.
I also recommend enforcing pep8 test along with tox by using something like flake8.

Raising this issue to discuss these three things.

@brohrer
Copy link
Owner

brohrer commented Sep 26, 2018

@markroxor these sound like solid recommendations for taking the code a step closer to production quality. I agree that we should implement them at some point, but I suggest that we hold off for now. Here is my thinking--please push back where you suspect I'm mistaken.

Writing a full set of tests, automating them, and implementing continuous integration will take a significant amount of effort and maintenance. The benefits will be greater the larger the codebase gets and the larger the user base grows. Our current user base is pretty small :) My sense is that we are not yet on the part of the cost-benefit trade-off to make it worthwhile. Reviewing each others code and encouraging contributors to use flake8 might be a reasonable interim solution.

Another aspect to consider is that the underlying algorithms have been changing significantly with every version, and will probably continue to do so. This makes it even tougher to maintain testing structure.

I don't throw this out to close the conversation, but to encourage it. What are your thoughts?

@markroxor
Copy link
Collaborator Author

I understand your point. It is not required right now but once the repository takes shape we will certainly need it.

I think we should keep this issue open and comeback to it in the future.

Meanwhile how about we use Travis CI just to ensure that pep8 is followed?

@brohrer
Copy link
Owner

brohrer commented Sep 26, 2018

Brilliant idea! I've added you as a collaborator so that you can make and test the necessary changes more easily without me slowing you down. I've enabled Travis CI for all the becca repositories.

BTW, I have absolutely no objection to adding tests, and even automating them with tox, if you feel motivated to do so. I would totally support that. But for now I feel comfortable not committing to 100% test coverage.

@markroxor
Copy link
Collaborator Author

Thank you Brandon. :)

@markroxor
Copy link
Collaborator Author

I have added the travis config and tox with flake8 - the build is passing on my fork. Unfortunately I cant see it active on your Travis. Maybe you have to enable it from there. Please check,

@brohrer
Copy link
Owner

brohrer commented Oct 1, 2018

I think I have it enabled now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants