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.

Sign up for GitHub

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

Jump to bottom

Improve rose stem coverage #181

Merged
merged 6 commits into from
Oct 20, 2022
Merged

Improve rose stem coverage #181

merged 6 commits into from
Oct 20, 2022

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Sep 13, 2022 •
edited
Loading

These changes partially address #157

I have added some unit tests designed to cover holes in the functional testing for rose stem I translated from Rose 2019.

added @dpmatthews as a functional review WRT mocking subprocessed fcm commands.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to setup.cfg.
  • Test improvements - No change log, documentation change &c

@wxtim wxtim self-assigned this Sep 13, 2022
@wxtim wxtim marked this pull request as draft September 13, 2022 14:39
@wxtim wxtim force-pushed the 20220913T1539--improve_rose_stem_coverage branch 2 times, most recently from 631aa3a to 022688c Compare September 14, 2022 07:02
@wxtim wxtim mentioned this pull request Sep 14, 2022
Closed
@wxtim wxtim changed the title 20220913 t1539 improve rose stem coverage Improve rose stem coverage Sep 14, 2022
@wxtim wxtim force-pushed the 20220913T1539--improve_rose_stem_coverage branch 2 times, most recently from 395ac55 to fe36d88 Compare October 4, 2022 08:26
@wxtim wxtim force-pushed the 20220913T1539--improve_rose_stem_coverage branch from fe36d88 to 4876133 Compare October 12, 2022 12:57
@wxtim wxtim force-pushed the 20220913T1539--improve_rose_stem_coverage branch from 4876133 to 8d137ab Compare October 12, 2022 15:07
@wxtim wxtim added this to the 1.2.0 milestone Oct 12, 2022
wxtim
wxtim commented Oct 12, 2022
View reviewed changes
cylc/rose/stem.py

ret = {}
for line in output.splitlines():
if ":" not in line:
continue
key, value = line.split(":", 1)

if key and value:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dpmatthews - This is splitting lines from fcm loc-layout. Will it ever be false?

@wxtim
Copy link
Member Author

wxtim commented Oct 12, 2022 •
edited
Loading

Coverage identifies the following remaining holes:

  • 112,124,178 - Some string output from "events" and Exceptions is untested.
  • 283 - A possible guard against a subprocessed FCM command going wrong which never gets hit - after a quick chat with Dave decided to live with this.
  • 362 - I'd need to dig a lot further into FCM to meaningfully test this.
  • 491 - Splitting standard items from config doesn't happen if there is not exactly 1 = in them - Is this sensible?
  • 606 - This looks trivial - I'm not sure it needs testing, but it's easy to test if you want it to be. What do people think?

@wxtim wxtim marked this pull request as ready for review October 12, 2022 15:27
@wxtim wxtim removed the request for review from dpmatthews October 12, 2022 15:28
MetRonnie
MetRonnie requested changes Oct 13, 2022
View reviewed changes
setup.cfg Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from MetRonnie October 13, 2022 09:47
oliver-sanders
oliver-sanders reviewed Oct 13, 2022
View reviewed changes
tests/unit/test_rose_stem_units.py Outdated Show resolved Hide resolved
tests/unit/test_rose_stem_units.py Outdated Show resolved Hide resolved
tests/unit/test_rose_stem_units.py Outdated Show resolved Hide resolved
MetRonnie
MetRonnie approved these changes Oct 13, 2022
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this went over my head but good to see coverage up to 94%

tests/unit/test_rose_stem_units.py Outdated Show resolved Hide resolved
@wxtim
response to review
2952550
@wxtim
Merge branch '20220913T1539--improve_rose_stem_coverage' of github.co…
3140bbc 
…m:wxtim/cylc-rose into 20220913T1539--improve_rose_stem_coverage

* '20220913T1539--improve_rose_stem_coverage' of github.com:wxtim/cylc-rose:
  Update setup.cfg
@wxtim
Copy link
Member Author

wxtim commented Oct 13, 2022

Most of this went over my head but good to see coverage up to 94%

Some of the methods which act as interfaces to FCM are pretty opaque to me, and there are at least 2 lines where I had to put in considerable effort to ensure that there wasn't a bug.

@oliver-sanders oliver-sanders merged commit 1012cf0 into cylc:master Oct 20, 2022
@wxtim wxtim deleted the 20220913T1539--improve_rose_stem_coverage branch December 1, 2022 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MetRonnie MetRonnie MetRonnie approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@wxtim wxtim

Labels
None yet
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@wxtim @oliver-sanders @MetRonnie