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.

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

Remove subprocess from tests #194

Merged

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Nov 16, 2022

Lots of tests are reliant on subprocess to run Cylc Command line scripts. In many cases this is not now necessary - Cylc offers an API for running the same script. Some of these may not work correctly until after Cylc VIP is merged

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.
  • Does not need tests - this is itself a testing change - the content of the tests should not have changed.
  • No change log entry required - testing change only
  • No documentation update required.

@wxtim wxtim added this to the 1.2.0 milestone Nov 16, 2022
@wxtim wxtim requested review from datamel and MetRonnie November 16, 2022 14:01
@wxtim wxtim self-assigned this Nov 16, 2022
@wxtim wxtim marked this pull request as draft November 16, 2022 14:04
@wxtim wxtim marked this pull request as ready for review November 18, 2022 09:49
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Nice speedup! I have run the tests before and after to make sure no tests were accidentally removed

Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

This looks great, no problems spotted from me. Thanks @wxtim.

@wxtim wxtim merged commit afc5f63 into cylc:master Nov 24, 2022
@wxtim wxtim deleted the 20221111T1526--master--remove_subprocess_from_tests branch November 24, 2022 13:49
wxtim added a commit to wxtim/cylc-rose that referenced this pull request Nov 25, 2022
…mental_improvements_to_testing

* 'master' of github.com:cylc/cylc-rose:
  Remove subprocess from tests (cylc#194)
  update stem - install interface (cylc#193)
  Update tests/unit/test_rose_stem_units.py
  response to review
  Update setup.cfg
  removed never used option
  Improve test coverage for Rose Stem
  reinstall changes to `rose-suite.conf` [tests] (cylc#178)
  Changed the rose stem functional tests to a more pytest-integration style.
  Bump rose dependency to `2.1.*`
  Bump dev version on master to next minor release (cylc#175)
  Fix changelog conflict with 1.1.x (cylc#186)
  Prepare release: 1.1.1 (cylc#183)
  Rose Stem Fixes (cylc#172)
  Don't pass rose variables with state ! or !! to Cylc. (cylc#171)
  Don't pass rose variables with state ! or !! to Cylc. (cylc#171)
  Bump cylc-flow dependency (cylc#173)
  functional tests only cleanup if succeeded
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants