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

Test -S/--rose-template-variable #310

Closed
wants to merge 3 commits into from
Closed

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Apr 12, 2024

Tests for cylc/cylc-flow#6065

Full explanation in that PR.

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 both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • CHANGES.md entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim self-assigned this Apr 12, 2024
@wxtim wxtim added this to the 1.4.0 milestone Apr 12, 2024
full test replicating original case
cylc_install_cli, cylc_reinstall_cli, tmp_path, file_poll,
provide_template_vars_workflow, purge_workflow, cylc_stop
):
"""When we restart a workflow, the play CLI options are honoured.
Copy link
Member Author

Choose a reason for hiding this comment

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

@oliver-sanders - I've included this to demonstrate that I think that the clearance of the arguments shouldn't happen if the reload is one that comes with a restart since restart can have options set, unlike reload, and that if they are set then the user would expect the CLI variables to override those in the rose files, even if we have just reinstalled too.

@MetRonnie MetRonnie changed the title Fix.5968.2 Test -S/--rose-template-variable Apr 12, 2024
"""Cylc stop & check the contact file"""
def _inner(id_):
run(split(f'cylc stop --now --now {id_}'))
contactfile = Path.home() / f'cylc-run{id_}.service/contact'
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be?

Suggested change
contactfile = Path.home() / f'cylc-run{id_}.service/contact'
contactfile = Path.home() / f'cylc-run/{id_}/.service/contact'

Or better yet

from cylc.flow.workflow_files import get_contact_file_path

Comment on lines +37 to +46
(tmp_path / 'flow.cylc').write_text(
'#!jinja2\n'
'[scheduling]\n'
' [[graph]]\n'
' R1 = foo\n'
'[runtime]\n'
' [[foo]]\n'
' script = cylc message -- {{var}}')
(tmp_path / 'rose-suite.conf').write_text(
'[template variables]\nvar="rose-suite.conf"')
Copy link
Member

Choose a reason for hiding this comment

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

N.B. I think it would be easier and cleaner to use a triple quoted string and textwrap.dedent()

Comment on lines +16 to +28
"""Functional tests for reinstalling of config files.
This test does the following:

1. Validates a workflow.
2. Installs a workflow with some opts set using -O and
ROSE_SUITE_OPT_CONF_KEYS.
3. Re-install workflow.
4. After modifying the source ``rose-suite.conf``, re-install the flow again.

At each step it checks the contents of
- ~/cylc-run/temporary-id/rose-suite.conf
- ~/cylc-run/temporary-id/opt/rose-suite-cylc-install.conf
"""
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem accurate. Copied from another test?

Copy link
Member

Choose a reason for hiding this comment

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

What about cylc vr?

@wxtim
Copy link
Member Author

wxtim commented Apr 15, 2024

Closing becuase after an offline discussion it was decided to approach a totally different solution.

@wxtim wxtim closed this Apr 15, 2024
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.

2 participants