Closed xuhang57 closed 6 years ago
coveralls
commented
6 years ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| hil/ext/switches/brocade.py | 0 | 3 | 0.0% | ||
| <!-- | Total: | 0 | 3 | 0.0% | --> |
| Totals | |
|---|---|
| Change from base Build 1641: | -0.02% |
| Covered Lines: | 2596 |
| Relevant Lines: | 6750 |
xuhang57
commented
6 years ago hmm, I don't know whether I should add some test cases to test the lines I just added. Since we are not testing the coverage on actual drivers, which makes sense that I am adding a few lines that are not covered.
What do you guys think? If I do need to add the tests, any idea on what kind of tests I should have would be really helpful!
naved001
commented
6 years ago If I do need to add the tests, any idea on what kind of tests I should have would be really helpful!
Just spitballing here:
tests/deployment/vlan_networks.py, add another test which simply connects a network to that switchport. This should fail and then you can catch the exception, and verify related things.But I don't know what are we testing here. The mechanism is already tested by tests/unit/deferred.py using the mock switch.
what do you think @zenhack
zenhack
commented
6 years ago I don't feel terribly strongly about adding tests for this. It might be nice to tweak the tests so we can run them against any driver instead of just mock, but that's a bigger change that I don't think it's worth holding this up over. @naved001 it might be good to run the deployment tests against the brocade switch if you've got the bandwidth for that.
I'm confused as to why coveralls is complaining; we marked the brocade driver as omit. it's also displaying paths in the virtualenv, which is weird. We've got some kind of configuration problem I think.
GitHub is telling me that I can still hit merge if I want to, so maybe we can just deal with coveralls config problem separately.
naved001
commented
6 years ago @naved001 it might be good to run the deployment tests against the brocade switch if you've got the bandwidth for that.
Yeah I can do that, but to be honest it doesn't really change much, if an error is raised it will also log it as an error and the tests will fail since we have the fail on log warnings. I'll run it anyway.
GitHub is telling me that I can still hit merge if I want to, so maybe we can just deal with coveralls config problem separately.
Yeah, coveralls is optional right now (unlike Travis, which is required). So we can ignore it as per our convenience.
xuhang57
commented
6 years ago But, yes, I am also very confused on why the coverage drops. I don't have a clue yet.
The thing is, you cannot drop the overage percentage when you have something that has already been 0.0%.
F = file
F1 (0.0%) + F2 + F3 + F4
------------------------- = 38.4%
Files@xuhang57, remember, you're making the uncovered file bigger, so that is going to drop the percentage of the overall code base. My question is more "why isn't coveralls ignoring this file?"
xuhang57
commented
6 years ago @zenhack Make sense.
I am basically following this: http://coverage.readthedocs.io/en/coverage-4.2/source.html#source
Would be nice if there is a second person that can read this doc a bit and make sure it suits what we need.
Currently, coveralls basically set certain files to be 0.0% rather than ignoring them I believe.
naved001
commented
6 years ago @zenhack Deployment tests passed with a caveat (unrelated to this PR):
for vlan_networks.py deployment tests if I specify a range of vlans in testsuite.cfg then the switch returns a range of vlans for which I have pushed a fixed in #958. The simple workaround was to specify non contiguous VLANs.
zenhack
commented
6 years ago LGTM. I'm going to merge, even though coveralls is unhappy (we've discussed the issue enough I think).
Since #929 introduces an approach of handling switch errors, this PR updates the existing switch drivers.
Relates to: https://github.com/CCI-MOC/hil/issues/948