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

cylc-rose: cylc list compatibility #65

Merged
merged 3 commits into from
Jul 15, 2021
Merged

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Jul 6, 2021

Partial Response to cylc/cylc-flow#4288
Twin of cylc/cylc-flow#4293

Test to ensure that cylc list works correctly with Cylc Rose options.

Do not expect tests to pass without twin PR.

@wxtim wxtim added this to the 1.0.0 milestone Jul 6, 2021
@wxtim wxtim self-assigned this Jul 6, 2021
@wxtim wxtim marked this pull request as draft July 6, 2021 19:07
@wxtim wxtim changed the title Test for upgrade cylc list Cylc Rose - Add Cylc List Jul 8, 2021
@wxtim wxtim changed the title Cylc Rose - Add Cylc List cylc-rose: cylc list compatibility Jul 8, 2021
@wxtim wxtim requested a review from datamel July 12, 2021 09:27
@wxtim wxtim marked this pull request as ready for review July 12, 2021 09:29
@wxtim wxtim requested a review from kinow July 12, 2021 13:25
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Only tests changed here, change looks OK to me.

The CI errors show 6 failed tests.

=========================== short test summary info ============================
FAILED tests/functional/test_pre_configure.py::test_cylc_script[[list] use -O]
FAILED tests/functional/test_pre_configure.py::test_cylc_script[[list] use -D]
FAILED tests/functional/test_pre_configure.py::test_cylc_script[[graph] use -O]
FAILED tests/functional/test_pre_configure.py::test_cylc_script[[graph] use -D]
FAILED tests/functional/test_pre_configure.py::test_cylc_script[[config] use -O]
FAILED tests/functional/test_pre_configure.py::test_cylc_script[[config] use -D]
================== 6 failed, 143 passed, 1 skipped in 33.16s ===================

I installed cylc-flow using pip install . in the cylc-flow dir (checked out your PR branch first). Running pytest it's failing to import cylc.flow, but if I comment out the pytest.ini extra options, and use python -m pytest it works (probably some sys.path confusion, perhaps related to namespaces…).

Then I still get 2 errors locally.

============================================================== short test summary info ===============================================================
FAILED tests/functional/test_pre_configure.py::test_validate[01_empy-None-None] - assert 1 == 0
FAILED tests/functional/test_pre_configure.py::test_process[01_empy-None-None] - assert '[meta]\n    ...    [[fin]]\n' == ''
===================================================== 2 failed, 147 passed, 1 skipped in 23.28s ======================================================

Are these two tests passing for you @wxtim?

Test output
====================================================================== FAILURES ======================================================================
__________________________________________________________ test_validate[01_empy-None-None] __________________________________________________________

tmp_path = PosixPath('/tmp/pytest-of-kinow/pytest-1/test_validate_01_empy_None_Non0')
srcdir = PosixPath('/home/kinow/Development/python/workspace/cylc-rose/tests/functional/01_empy'), envvars = None, args = None

    @pytest.mark.parametrize(
        'srcdir, envvars, args',
        [
            ('00_jinja2_basic', None, None),
            ('01_empy', None, None),
            ('02_env', None, None),
            (
                '04_opts_set_from_env',
                {'ROSE_SUITE_OPT_CONF_KEYS': 'Gaelige'},
                None
            ),
            (
                '05_opts_set_from_rose_suite_conf',
                {'ROSE_SUITE_OPT_CONF_KEYS': ''},
                None
            ),
            ('06_jinja2_thorough', {'XYZ': 'xyz'}, None),
            ('07_cli_override', {'XYZ': ''}, ["--set=CLI_VAR='Wobble'"]),
            ('09_template_vars_vanilla', {'XYZ': 'xyz'}, None),
        ],
    )
    def test_validate(tmp_path, srcdir, envvars, args):
        if envvars is not None:
            envvars = os.environ.update(envvars)
        srcdir = Path(__file__).parent / srcdir
        script = ['cylc', 'validate', str(srcdir)]
        if args:
            script = script + args
>       assert (
            run(script, env=envvars)
        ).returncode == 0
E       assert 1 == 0
E         +1
E         -0

tests/functional/test_pre_configure.py:94: AssertionError
---------------------------------------------------------------- Captured stderr call ----------------------------------------------------------------
ParsecError: EmPy Python package must be installed to process file: /home/kinow/Development/python/workspace/cylc-rose/tests/functional/01_empy/flow.cylc
__________________________________________________________ test_process[01_empy-None-None] ___________________________________________________________

tmp_path = PosixPath('/tmp/pytest-of-kinow/pytest-1/test_process_01_empy_None_None0')
srcdir = PosixPath('/home/kinow/Development/python/workspace/cylc-rose/tests/functional/01_empy'), envvars = None, args = None

    @pytest.mark.parametrize(
        'srcdir, envvars, args',
        [
            ('00_jinja2_basic', None, None),
            ('01_empy', None, None),
            (
                '04_opts_set_from_env',
                {'ROSE_SUITE_OPT_CONF_KEYS': 'Gaelige'},
                None
            ),
            (
                '05_opts_set_from_rose_suite_conf',
                {'ROSE_SUITE_OPT_CONF_KEYS': ''},
                None
            ),
            ('06_jinja2_thorough', {'XYZ': 'xyz'}, None),
        ],
    )
    def test_process(tmp_path, srcdir, envvars, args):
        if envvars is not None:
            envvars = os.environ.update(envvars)
        srcdir = Path(__file__).parent / srcdir
        result = run(
            ['cylc', 'view', '-p', '--stdout', str(srcdir)],
            capture_output=True,
            env=envvars
        ).stdout.decode()
        expect = (srcdir / 'processed.conf.control').read_text()
>       assert expect == result
E       assert '[meta]\n    ...    [[fin]]\n' == ''
E         + [meta]
E         +     title = "Add empy vars from a rose-suite.conf"
E         +     description = """
E         +     Natively, in Cylc!
E         +     """
E         + [scheduling]
E         +     initial cycle point = 1...
E         
E         ...Full output truncated (24 lines hidden), use '-vv' to show

tests/functional/test_pre_configure.py:127: AssertionError
============================================================== short test summary info ===============================================================
FAILED tests/functional/test_pre_configure.py::test_validate[01_empy-None-None] - assert 1 == 0
FAILED tests/functional/test_pre_configure.py::test_process[01_empy-None-None] - assert '[meta]\n    ...    [[fin]]\n' == ''
===================================================== 2 failed, 147 passed, 1 skipped in 23.28s ======================================================

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

My bad, re-reading @wxtim 's comments on how to verify, and then looking at the text I pasted I can see it was missing empy. pip install empy, re-run tests, and all good now!

========================================================== 149 passed, 1 skipped in 22.74s ===========================================================

Approved.

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.

Looks great, thanks @wxtim. I have read the code and checked out the branch.

Only one thing which is unrelated to this PR but flagged for me when running this is, not sure if you think one day it would be worth (or possible) re-writing some of the tests here to bypass the global config? Had some errors relating to config from another development branch.

@datamel datamel merged commit 81d0b60 into cylc:master Jul 15, 2021
@oliver-sanders oliver-sanders modified the milestones: 1.0.0, 0.2 Jul 22, 2021
@wxtim wxtim deleted the cylc-rose.add-list branch December 1, 2022 13:09
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.

4 participants