run pep8 along with other scripts rather than before_script #148
Conversation
* without this the build will fail and stop if pep8 tests fail. We still want to see if there are any other failures in the travis build. This[link](https://docs.travis-ci.com/user/customizing-the-build#Breaking-the-Build) has more info. Thanks @jeremyfreudberg
|
@naved001 I personally feel this is a bad idea as the code might have bad indents which might fail the tests. I feel that the code should be sanitized before the tests are run. |
|
Hmmm, I feel like with one run we should know everything that's wrong with the code. Like for example in #147 we'll have to wait for the submitter to fix pep8 first and only then we get to see the what the rest of the results are. |
|
I understand what you mean, but in #147 there are indentation errors which will fail the tests. The submitter is supposed to run pep8 and some subset of unit tests before submitting PR. The CI is for defending master branch and pep8 is to defend from unnecessary test failures and ensure code quality. If a pep8 fail happens and some tests fail it may be due to the pep8 failures which will be difficult to tell. If we get pep8 out of the way then it is clear that the tests failed due to some other reason. |
|
I disagree completely. What about unit tests - if one of those fails, do you give up and cancel the integration tests? I don't think so. Pep8 as a prerequisite wastes the developer's time - personally I find pep8 to be an incredibly flawed standard and I would much rather be able to write my code in a natural way, get the results from the test suite, and then worry about code style later. There could be a compromise though - check for syntax errors and irreconcilable |
|
@jeremyfreudberg obviously unit tests will have to run along with integration tests. |
|
Yes we should use pylint, but it has lots of checks and can be noisy. We would have to enable one check at a time (because it will complain about almost everything given the condition of our code base). |
|
Lol I agree. I think there is an issue for that. Make sure you link it with this. |
|
short term solution is use goodies like flake8 and pylint can be added when you guys have time |
There was a problem hiding this comment.
LGTM. @naved001 squa 8000 sh commits and merge. I will look into the coveralls thing in another PR.
|
We should merge only after we do an RCA for the complaining coveralls. |
|
RCA ? |
|
Root Cause Analysis (RCA) |
|
What's root cause analysis and why's that blocking this PR? Can you elaborate? |
|
Do you want to solve the coveralls issues withing this PR then? |
To find the reason why coveralls is complaining
Only if the coveralls is complaining because of some changes made in this PR. Now that we are maintaining a single master branch, we should ensure that whenever we merge any PR, it should pass all the tests and any required sanity checks (like coveralls). Resolving coverall complains will not only improve our test case suite but help us in the longer run. |
Because we added some lines to .travis.yml so the overall coverage drops. Ideally, coveralls should ignore the travis file and docs when collecting coverage. We can't write tests for travil.yml But I see your point, we should first raise a PR to tell coveralls to ignore some files, and then merge this one. |
I see
Ok |
|
There is something called stage in travis CI now(this is beta but worked well for me). https://docs.travis-ci.com/user/build-stages. It does parallel execution of all jobs in one stage and won't go next stage until the current stage completes. |
|
Looked into this and it looks like it is related to rpc and not travis.yml. Coveralls only has coverage for the python files. For some reason the coverage of rpc is varying between builds. @naved001 you can merge this for now as the rebuild fixed the issue. I will say it is pointless to debug the RPC issue as we will be removing RPC in the redesign. @apoorvemohan what do you think ? |
|
@apoorvemohan ping |
|
@apoorvemohan ping |
@chemistry-sourabh Is it difficult to debug the RPC issue? At this point, it seems that re-design will take at least a couple of months to complete. I would suggest that we first understand the severity of the RPC bug before we decide to push the merge button on this. |
|
I found out the reason for bug. It is not related to some functionality issue. Sometimes during the build the RPC throws some connection exceptions and sometimes it doesn't. I wrote it in such a way so that it will recover, but due to it the coverage changes as different chunks of code is touched. |
|
@apoorvemohan @chemistry-sourabh I think we should simply ignore coveralls for now. I had configured this repo to make the merge button green only when:
coveralls is not required. We can make coveralls mandatory when we understand how that works better. The reason why I want to push this because it is absurd having to fix all pep8 errors (it's just style after-all) first before getting to see the actual problems with your code. |
|
kinda weird, but coveralls is happy now with this PR lol. |
|
@pns005 thinks that coveralls is a good tool and that we shouldn't drop it. She said that you can make an exception for this PR; but coveralls is happy with this PR anyway. |
|
+1 |
to see if there are any other failures in the travis build so this would keep it going.
This link has more info. Thanks @jeremyfreudberg