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
[JUJU-928] rewrite to bash nw charm storage test #14545
[JUJU-928] rewrite to bash nw charm storage test #14545
Conversation
e9b86e5
to
2553a66
Compare
df6c261
to
57a8b9a
Compare
57a8b9a
to
dc67a64
Compare
@@ -0,0 +1,142 @@ | |||
run_charm_storage() { |
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.
Need to add a comment that briefly describes what this test does.
run_charm_storage() { | ||
echo | ||
|
||
model_name="test-storage" |
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.
here we need to use ensure
see the other examples:
ensure "test-deploy-charm" "${file}" |
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 had it then removed it because I noticed we are already creating a model in the task.sh
when we run bootstrap ${model_name} ${file}
. ensure "${model_name}" "${file}"
then again creates another model. And since this test only has one subtest. I reckon that there is no need to create another model instead it can just use the already created model in task.sh
test-storage()
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.
EDIT: I had not considered there might be need to run this subtest individually hence the need for ensure
. Thank you for catching this. Will add it.
wait_for "{}" ".applications" | ||
|
||
# Assess charm storage with the filesystem storage provider | ||
|
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.
Do we need this empty line?
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.
We need to check for the {}
for if we do not the next storage unit number will increment since the already previous storage units have not been removed, which will tamper with the next assertion.
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.
EDIT: I will need to confirm this once more.
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 confirmed, without the wait_for
the test assertions fail why? Juju make block storages persistent and when you deploy with an existing persistent storage unit, then the next storage unit is incremented, hence the test assertion will fail. Yes I can use remove-storage
, but that is not how the python test am replacing is currently implemented as it is does not test for 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.
I was talking about "empty line (number: 47)" :) not line with wait_for.
To wait_for
I've no doubt.
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.
Removed the newline.
juju remove-application dummy-storage-mp | ||
wait_for "{}" ".applications" | ||
echo "All charm storage tests PASSED" | ||
|
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.
when you will add ensure
, don't forget to destroy_model
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.
Fixed.
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.
lg2m
9209a28
to
e2db5a9
Compare
e2db5a9
to
b01db70
Compare
/merge |
#14595 ``` Merge branch '2.9' into 2.9-into-develop # Conflicts: # acceptancetests/assess_cloud.py # acceptancetests/assess_endpoint_bindings.py # acceptancetests/assess_heterogeneous_control.py # state/migration_import_test.go # apiserver/facades/client/client/status.go # scripts/win-installer/setup.iss # snap/snapcraft.yaml # version/version.go ``` - #14551 - #14557 - #14560 - #14558 - #14554 - #14570 - #14577 - #14506 - #14538 - #14559 - #14574 - #14573 - #14567 - #14545 - #14541 - #14588
This PR is part of the effort of moving from python tests to bash based test. Here is the old python test
Checklist
If an item is not applicable, use
~strikethrough~
.[ ] Go unit tests, with comments saying what you're testing[ ] doc.go added or updated in changed packagesQA steps
The tests should pass
cd tests ./main.sh -v -p aws storage run_charm_storage