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

tests/i: add reflog fixture #5826

Merged
merged 5 commits into from
Dec 14, 2023
Merged

tests/i: add reflog fixture #5826

merged 5 commits into from
Dec 14, 2023

Conversation

oliver-sanders
Copy link
Member

Add a reflog fixture to the integration tests for trigger testing.

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
Copy link
Member

wxtim commented Nov 23, 2023

Possible counter-example:

async def test_reflog(flow, scheduler, run, reflog, complete):
    id_ = flow({
        'scheduler': {
            'allow implicit tasks': 'true'
        },
        'scheduling': {
            'graph': {
                'R1': 'foo:failed => bar'
            }
        },
    })
    schd = scheduler(id_, paused_start=False)

    async with run(schd):
        triggers = reflog(schd)
        await complete(schd)
    breakpoint(header=f'{triggers=}')
    assert triggers == {
        ('1/foo', None),
        ('1/bar', ('1/foo',)),   # This may well not be right, but we're not getting any trigger at all here
    }

@oliver-sanders
Copy link
Member Author

This example should hang because you haven't configured foo to fail.

Try setting [runtime][foo][simulation]fail cycle points = all.

@wxtim
Copy link
Member

wxtim commented Nov 23, 2023

This example should hang because you haven't configured foo to fail.

Try setting [runtime][foo][simulation]fail cycle points = all.

Try task:started too?

@oliver-sanders oliver-sanders requested review from MetRonnie and removed request for hjoliver November 23, 2023 12:37
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

.github/workflows/test_fast.yml Outdated Show resolved Hide resolved
tests/integration/conftest.py Show resolved Hide resolved
@oliver-sanders oliver-sanders force-pushed the reflog branch 2 times, most recently from 6cc9477 to 2b9476e Compare November 28, 2023 10:57
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.

This looks like it's made the TUI integration tests flaky? At least, getting different failures on each python version

.github/workflows/test_fast.yml Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Nov 28, 2023

Would be very surprised if chaing TZ made Tui tests unstable? Will take a look at them once I've cleared my bug backlog.

oliver-sanders and others added 4 commits December 13, 2023 10:58
* Add an itegration test pattern for implementing reference tests.
* We were using the pytest-env plugin to run the tests in a non-UTC time
  zone.
* The pytest-env plugin doesn't work with pytest-xdist so this was being
  ignored.
* Also due to the way TZ support works in Python, changing the env var
  whilst Python is running may or may not result in changes.
@oliver-sanders
Copy link
Member Author

oliver-sanders commented Dec 13, 2023

Got to the bottom of the test failure, it's caused by a bug in simulation mode fixed on #5721

I've put in a temporary patch which can be removed in or after that PR depending on merge order.

# https://github.com/cylc/cylc-flow/pull/5721
'root': {
'simulation': {
'default run length': 'PT1M',
Copy link
Member Author

Choose a reason for hiding this comment

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

The patch, short runtimes could cause tasks to go through running & succeeded even whilst the workflow is paused.

@@ -3,11 +3,11 @@
<span style="color:#000000;background:#e5e5e5">│</span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">Path: mypath </span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">│</span>
<span style="color:#000000;background:#e5e5e5">│</span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">&lt;</span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#e5e5e5;background:#000000">S</span><span style="color:#000000;background:#e5e5e5">elect File </span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">&gt;</span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">│</span>
<span style="color:#000000;background:#e5e5e5">│</span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">│</span>
<span style="color:#000000;background:#e5e5e5">│</span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">[runtime] </span><span style="color:#000000;background:#e5e5e5"> </span><span style="color:#000000;background:#e5e5e5">│</span>
Copy link
Member Author

Choose a reason for hiding this comment

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

The order of the [scheduling] and [runtime] sections of the config changed as a result of prepending the default simulation runtime.

@oliver-sanders
Copy link
Member Author

Manylinux failures are unrelated, see this run on master: https://github.com/cylc/cylc-flow/actions/runs/7197952698

@oliver-sanders oliver-sanders marked this pull request as ready for review December 14, 2023 10:02
@MetRonnie MetRonnie merged commit 6f62691 into cylc:master Dec 14, 2023
30 of 36 checks passed
@oliver-sanders oliver-sanders deleted the reflog branch December 14, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants