run pep8 along with other scripts rather than before_script

[naved001]

• without this the build will fail and stop if pep8 tests fail. but we still want 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

[chemistry-sourabh]

@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.

[naved001]

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.

[chemistry-sourabh]

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.

[jeremyfreudberg]

@chemistry-sourabh

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 syntax indentation errors in the before_script, and move main pep8 check into script.

[chemistry-sourabh]

@jeremyfreudberg obviously unit tests will have to run along with integration tests. I don't have an issue with your suggestion. @naved001 probably you can use pylint to check for syntax errors in before_script and run another pylint with only style checkin ?

[naved001]

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).

[chemistry-sourabh]

Lol I agree. I think there is an issue for that. Make sure you link it with this.

[jeremyfreudberg]

short term solution is use python -m py_compile $filename and it will check syntax, then you can just move the existing pep8 command to script

goodies like flake8 and pylint can be added when you guys have time

[apoorvemohan]

We should merge only after we do an RCA for the complaining coveralls.

[chemistry-sourabh]

RCA ?

[apoorvemohan]

Root Cause Analysis (RCA)

[naved001]

What's root cause analysis and why's that blocking this PR? Can you elaborate?

[naved001]

Do you want to solve the coveralls issues withing this PR then?

[apoorvemohan]

What's root cause analysis and why's that blocking this PR? Can you elaborate?

To find the reason why coveralls is complaining

Do you want to solve the coveralls issues withing this PR then?

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.

[naved001]

To find the reason why coveralls is complaining

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.

[apoorvemohan]

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.

I see

But I see your point, we should first raise a PR to tell coveralls to ignore some files, and then merge this one.

Ok

[ravisantoshgudimetla]

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.

[chemistry-sourabh]

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 ?

[naved001]

@apoorvemohan ping @chemistry-sourabh if you guys thing it's okay to merge, then go ahead and press the squash and merge button on it.

[naved001]

@apoorvemohan ping

[apoorvemohan]

I will say it is pointless to debug the RPC issue as we will be removing RPC in the redesign.

@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.

[chemistry-sourabh]

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.

[naved001]

@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:

• someone with write access approves it

• and when it passes travis CI checks.

  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.

[apoorvemohan]

@pns005 What are your views on @naved001's suggestions?

[naved001]

kinda weird, but coveralls is happy now with this PR lol.

[naved001]

@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.

[chemistry-sourabh]

@apoorvemohan ?

[apoorvemohan]

+1