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

Queue warning c8 #3541

Merged
merged 5 commits into from
Mar 30, 2020
Merged

Queue warning c8 #3541

merged 5 commits into from
Mar 30, 2020

Conversation

hjoliver
Copy link
Member

These changes partially address #3470 (close after PR port to 7.8.x and 7.9.x as well)

master port of #3539 (full description there)

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).

  • Appropriate tests are included (unit and/or functional).

  • Appropriate change log entry included.

  • No documentation update required. (No significant change to users, just fewer warnings)

@hjoliver hjoliver added this to the cylc-8.0a2 milestone Mar 30, 2020
@hjoliver hjoliver requested review from kinow and wxtim March 30, 2020 02:05
@hjoliver hjoliver self-assigned this Mar 30, 2020
@hjoliver
Copy link
Member Author

(Draft PR for now - I'll try to replace the functional test with a unit test).

@hjoliver hjoliver marked this pull request as ready for review March 30, 2020 03:54
@hjoliver
Copy link
Member Author

Ready for review. I replaced the functional test with a unit test. The same unit test probably could be used on 7.8/9.x as I didn't end up using the some master-only stuff that I thought I would. But, I think it doesn't matter. It'll be a non-flaky functional test for the old system.

kinow
kinow approved these changes Mar 30, 2020
View reviewed changes
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.

Approving as unit test passed already on Travis.

cylc/flow/tests/test_config.py Outdated Show resolved Hide resolved
cylc/flow/tests/test_config.py Show resolved Hide resolved
cylc/flow/tests/test_config.py Outdated
@@ -211,3 +212,32 @@ def test_family_inheritance_and_quotes(self):
['MAINFAM_major1_minor10'])
assert 'goodbye_0_major1_minor10' in \
config.runtime['descendants']['SOMEFAM']


def test_queue_config(caplog):
Copy link
Member

Choose a reason for hiding this comment

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

Today-I-Learned about caplog 🎉 thanks!

https://docs.pytest.org/en/latest/logging.html#caplog-fixture

wxtim
wxtim reviewed Mar 30, 2020
View reviewed changes
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

  • Read and understood
  • Tried on a suite of my own
  • Run tests using pytest
  • Travis runs

Concerned that Travis is failing - have restarted the job.

@wxtim wxtim self-requested a review March 30, 2020 11:39
wxtim
wxtim approved these changes Mar 30, 2020
View reviewed changes
Copy link
Member

@wxtim wxtim 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.

Previous comment was to be an approval contingent on the tests passing.
I'll merge if Travis works on re-run.

@oliver-sanders
Copy link
Member

Kicked tests.

@kinow
Copy link
Member

kinow commented Mar 30, 2020

Tests passed on Travis after two kicks. Two approvals, ready to be merged.

@kinow kinow merged commit 3b2d9d9 into cylc:master Mar 30, 2020
@hjoliver
Copy link
Member Author

Thanks @kinow

I was just looking at the test that failed: cylc-scan/02-sigstop.t. Surprisingly it failed for me locally, on this branch and master, but then it started passing after I deleted the entire cylc-run directory. So there might be some interference from other test directories, somehow. I'll see if there's any obvious problem...

@hjoliver hjoliver deleted the queue-warning-c8 branch March 30, 2020 21:09
@hjoliver
Copy link
Member Author

(Nope, can't see anything wrong with the test, will have to leave it as a mystery for now).

@hjoliver hjoliver mentioned this pull request Mar 31, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kinow kinow kinow approved these changes

@wxtim wxtim wxtim approved these changes

Assignees

@hjoliver hjoliver

Labels
None yet
Projects
None yet
Milestone
cylc-8.0a2
Development

Successfully merging this pull request may close these issues.
4 participants
@hjoliver @oliver-sanders @kinow @wxtim