-
Notifications
You must be signed in to change notification settings - Fork 11
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
100% cov #196
100% cov #196
Conversation
0a051e5
to
652ef81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯!
Great, glad to have full coverage for at least one of our repos.
tests/conftest.py
Outdated
@pytest.fixture | ||
def cylc_install_cli(capsys, caplog): | ||
return _cylc_install_cli(capsys, caplog) | ||
return _cylc_cli( | ||
capsys, caplog, | ||
cylc_install, install_gop | ||
) | ||
|
||
|
||
@pytest.fixture(scope='module') | ||
def mod_cylc_install_cli(mod_capsys, mod_caplog): | ||
return _cylc_install_cli(mod_capsys, mod_caplog) | ||
return _cylc_cli( | ||
mod_capsys, mod_caplog, | ||
cylc_install, install_gop | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe do this more similarly to the Cylc integration tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be too long ago, and I'm not terribly familiar with the Cylc Integration tests... but...
My impression is that they largely revolve around running flows which until recently were written to the flow directory.
Here most of the tests revolve around the install process, so I'm not sure that the same approach makes sense.
Even if it does, would you mind me (or you) kicking it into a new issue. I've had this PR open a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my point was that this implementation differs from the one you are currently adding in cylc-flow with the cylc vr
pull request. It would make sense to do it the same way for both.
@oliver-sanders - this one is by no means a priority, but I think it's worth getting in at some point. |
- Remove now superflous checks - Refactored Conftest to allow use of Cylc view - Added duplicate line in workflow db to check retrieval works correctly.
…tion use install-cylc-components action
- Remove now superflous checks - Refactored Conftest to allow use of Cylc view - Added duplicate line in workflow db to check retrieval works correctly.
Co-authored-by: Oliver Sanders <[email protected]>
…cov_to_100 * 'get_cov_to_100' of github.com:wxtim/cylc-rose: remove rose stem test reliance on Popen wips remove fake popen response to review 1: simple fixes Update .codecov.yml Update .codecov.yml 100% rose stem some work on rose stem fixed holes in the coverage of entry_points.py 100 Cover for utilities.py and platform_utils.py - Remove now superflous checks - Refactored Conftest to allow use of Cylc view - Added duplicate line in workflow db to check retrieval works correctly.
Thoroughly stale: I've made a note on my personal todo list that this branch is worth pulling stuff out of. |
Opened PR to allow commenting on code changes.
Requires cylc/cylc-flow#5246 to pass all tests
These changes close #157
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
.