-
Notifications
You must be signed in to change notification settings - Fork 94
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
More deterministic tests #3336
More deterministic tests #3336
Conversation
while test "$(grep -cF 'Reload completed' "${CYLC_SUITE_LOG_DIR}/log")" -ne "$1" | ||
do | ||
sleep 1 | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add some form of timeout to these tests prevent them from hanging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the poll
function from test_header
- it now aborts after a minute if condition not met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if there is an easy way to expose the test battery poll function in the job scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set PATH
at the suite level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No straightforward. It is a shell function in the test_header
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purpose of this change, we should have a way to share the shell function, because the pattern is repeated in many places. I wonder if we should just add the function to cylc/flow/etc/job.sh
? Maybe users can make use of a poll function in their task jobs as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are multiple test functions it might be useful to run from within a suite. How about moving the functions out of the test header into another file which can easily be sourced in a pre-script
or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a library of shell functions is definitely useful, but probably outside the scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New issue for this: #3346
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a function to cylc/flow/etc/job.sh
to allow job scripts that have the same poll suite log pattern to simple call a function. We can generalise this as part of #3346.
This is not done yet. I have yet to do capture:
|
5af74ca
to
98968e0
Compare
The tests are definitely more stable, but I am not really seeing any overall speed gain (although most of the heavily modified tests do run faster individually), so removing the faster bit from the title. |
98968e0
to
fa44e30
Compare
Tests very stable in my environment, but not so on Travis CI. I'll continue to monitor. |
1e71f28
to
58fe9aa
Compare
Is this ready to for a final review or are you still working on it? |
This is ready. (More work can be done in follow-on PRs.) |
try: | ||
assert get_unix_time_from_time_string(time_str) == time_sec | ||
finally: | ||
CALENDAR.set_mode(mode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change replaces tests/cylc.wallclock/00-single.t
#------------------------------------------------------------------------------- | ||
run_ok "${TEST_NAME_BASE}-cycling-iso8601" python3 -m 'cylc.flow.cycling.iso8601' | ||
run_ok "${TEST_NAME_BASE}-cycling-integer" python3 -m 'cylc.flow.cycling.integer' | ||
run_ok "${TEST_NAME_BASE}-cycling" python3 -c 'import cylc.flow.cycling' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I guess this used to actually run some primitive unit tests in the source modules, and we forgot to delete it once we started doing unit tests separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, no problems found 👍
Remove many sleeps in favour of detecting events. test_header: new function to install, validate and run reference test.
Dedicated functions for the usage patterns. Positive poll logic.
This is a temporary measure to address the repeated pattern in various test suites where we have to repeatedly poll the suite log for events.
58fe9aa
to
e4943d8
Compare
Re-based to pick up and test with latest changes. |
Remove many sleeps in favour of detecting events.
test_header: new function to install, validate and run reference test.
test_header: dedicated poll related functions to simplify usage patterns.
poll
function, as most usage were doingpoll ! ...
.New function to poll suite log in
cylc/flow/etc/job.sh
. Will follow in #3346.Remove unit tests duplicated in battery.
Move
cylc.wallclock
tests from battery to unit test.These changes partially address #3046.
Requirements check-list
CONTRIBUTING.md
and added my name as a Code Contributor.