-
Notifications
You must be signed in to change notification settings - Fork 93
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
Upgraded built-in suites to cylc7. #2233
Conversation
tests/validate/63-builtin-suites.t
Outdated
#out_file.write(re.sub(r'.*naked dummy tasks detected.*\n(\+\t.*\n)+', | ||
# '', in_file.read())) | ||
out_file.write(contents)" "$1" | ||
} |
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've had to filter out some messages for certain suites to pass. Are the "offsets are normally positive" messages necessary?
de163c3
to
aad7bf2
Compare
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 OK. Please consider addressing the style comments on bash.
tests/validate/63-builtin-suites.t
Outdated
#------------------------------------------------------------------------------- | ||
# Validate suites. | ||
for suite in ${SUITES[@]}; do | ||
suite_name=$(echo ${suite:$ABS_PATH_LENGTH} | sed 's/\//-/g') |
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.
Single command with no pipe:
suite_name=$(sed 's/\//-/g' <<<"${suite:$ABS_PATH_LENGTH}")
tests/validate/63-builtin-suites.t
Outdated
SUITES= | ||
for path in ${PATHS[@]}; do | ||
SUITES+=($(find "${CYLC_DIR}/${path}" -name 'suite.rc')) | ||
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.
Can be a single command?
SUITES="$(find "${CYLC_DIR}/"{examples,dev/suites} -name 'suite.rc')"
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.
Didn't know you could do that, nice!
tests/validate/63-builtin-suites.t
Outdated
suite_name=$(echo ${suite:$ABS_PATH_LENGTH} | sed 's/\//-/g') | ||
TEST_NAME="${TEST_NAME_BASE}${suite_name}" | ||
run_ok "${TEST_NAME}" cylc validate "${suite}" -v -v | ||
filter_warnings "${TEST_NAME}.stderr" |
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.
Can this just be a sed -i
? E.g.:
sed -i '
/naked dummy tasks detected/d;
/clock-\(trigger\|expire\) offsets are normally positive/d;
' "${TEST_NAME}.stderr"
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.
naked dummy tasks detected
messages are followed by + <task name>
lines which have to be filtered out as well. I couldn't workout how to perform a suitable multi-line sed deletion so went with python.
aad7bf2
to
693c922
Compare
@oliver-sanders - do we still need |
Nope. |
Our built-in examples were getting out of date, this adds a validation check to the test battery.