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

[JUJU-1195]rewrite to bash based nw clouds display test #14455

Merged
merged 3 commits into from
Aug 16, 2022
Merged

[JUJU-1195]rewrite to bash based nw clouds display test #14455

merged 3 commits into from
Aug 16, 2022

Conversation

basebandit
Copy link
Contributor

@basebandit basebandit commented Aug 11, 2022 •
edited

This is part of the effort to migrate python based tests to bash based tests for ease of maintenance and simplicity.

Checklist

  • Code style: imports ordered, good names, simple structure, etc
  • Comments saying why design decisions were made
  • [ ] Go unit tests, with comments saying what you're testing
  • Integration tests, with comments saying what you're testing
  • [ ] doc.go added or updated in changed packages

QA steps

cd tests
make build && make install
./main.sh cli run_clouds
./main.sh cli run_show_cloud

@basebandit basebandit force-pushed the JUJU-1195_Rewrite_to_bash_based_nw-clouds-display_test branch from 131d584 to d148257 Compare August 11, 2022 14:19
@anvial
Copy link
Member

anvial commented Aug 12, 2022

QA steps passed successfully: https://pastebin.ubuntu.com/p/jKHXYNj7dd/

There are some comments to code.

anvial
anvial requested changes Aug 12, 2022
View reviewed changes
tests/suites/cli/display_clouds.sh
@@ -45,6 +45,107 @@ EOF
fi
}

# Assess how clouds behaves when only expected clouds are defined.
run_assess_clouds() {
Copy link
Member

Choose a reason for hiding this comment

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

need to rename: run_clouds

tests/suites/cli/display_clouds.sh Outdated
@@ -45,6 +45,107 @@ EOF
fi
}

# Assess how clouds behaves when only expected clouds are defined.
Copy link
Member

Choose a reason for hiding this comment

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

Change according to renaming

tests/suites/cli/display_clouds.sh Outdated
}

# Assess how show-cloud behaves
run_assess_show_cloud() {
Copy link
Member

Choose a reason for hiding this comment

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

need to rename: "run_show_cloud"

tests/suites/cli/display_clouds.sh
echo

mkdir -p "${TEST_DIR}/juju"
echo "" >>"${TEST_DIR}/juju/public-clouds.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

I do not think we need to create a subfolder juju. Other tests writes directly to TEST_DIR

tests/suites/cli/display_clouds.sh
#Remove the attributes added by display. i.e 'defined' attribute whose value is not "built-in".
CLOUD_LIST=$(JUJU_DATA="${TEST_DIR}/juju" juju clouds --client --format=json 2>/dev/null | jq 'with_entries(select(.value.defined != "built-in"))')
EXPECTED={}
if [ "${CLOUD_LIST}" != "${EXPECTED}" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

for this purpose we use check, check_contains or diff (see /includes/check.sh).

anvial
anvial requested changes Aug 12, 2022
View reviewed changes
tests/suites/cli/display_clouds.sh Outdated
mkdir -p "${TEST_DIR}/juju"
echo "" >>"${TEST_DIR}/juju/public-clouds.yaml"
echo "" >>"${TEST_DIR}/juju/credentials.yaml"
mkdir -p "${TEST_DIR}"
Copy link
Member

Choose a reason for hiding this comment

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

I think that we do not need to mkdir, look to other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned up.

anvial
anvial requested changes Aug 12, 2022
View reviewed changes
tests/suites/cli/display_clouds.sh Outdated
exit 1
fi
OUT=$(JUJU_DATA="${TEST_DIR}" juju clouds --all --format=json 2>/dev/null | jq '.[] | select(.defined != "built-in")')
check_ge "${OUT}" "${EXPECTED}"
Copy link
Member

Choose a reason for hiding this comment

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

looks like better to you check_contains or diff

anvial
anvial approved these changes Aug 15, 2022
View reviewed changes
Copy link
Member

@anvial anvial left a comment

Choose a reason for hiding this comment

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

lg2m

@basebandit basebandit force-pushed the JUJU-1195_Rewrite_to_bash_based_nw-clouds-display_test branch 2 times, most recently from 61f8035 to cea93dc Compare August 15, 2022 11:51
@basebandit basebandit force-pushed the JUJU-1195_Rewrite_to_bash_based_nw-clouds-display_test branch from cea93dc to b4acd28 Compare August 16, 2022 09:09
@basebandit
Copy link
Contributor Author

/merge

1 similar comment
@jack-w-shaw
Copy link
Member

/merge

@jujubot jujubot merged commit a63829b into juju:2.9 Aug 16, 2022
@ycliuhw ycliuhw mentioned this pull request Aug 19, 2022
Merged
jujubot added a commit that referenced this pull request Aug 20, 2022
@jujubot
Merge pull request #14488 from ycliuhw/2.9-into-develop
edf5703 
#14488

```
Merge branch '2.9' into develop

# Conflicts:
# .github/workflows/smoke.yml
# apiserver/common/tools_test.go
# apiserver/facades/client/application/application_unit_test.go
# apiserver/facades/client/application/mock_test.go
# apiserver/facades/client/application/package_test.go
# apiserver/facades/client/cloud/cloudV2_test.go
# apiserver/facades/client/cloud/cloud_test.go
# caas/kubernetes/provider/application/application.go
# cmd/juju/commands/upgradecontroller_test.go
# cmd/juju/commands/upgrademodel.go
# cmd/juju/commands/upgrademodel_test.go
# cmd/juju/controller/kill.go
# go.mod
# go.sum
# scripts/win-installer/setup.iss
# snap/snapcraft.yaml
# tests/suites/deploy/bundles/lxd-profile-bundle.yaml
# tests/suites/deploy/bundles/telegraf_bundle.yaml
# tests/suites/deploy/bundles/trusted_bundle.yaml
# tests/suites/deploy/deploy_charms.sh
# tests/suites/network/network_health.sh
# tests/suites/ovs_maas/ovs_netplan_config.sh
# tests/suites/spaces_ec2/juju_bind.sh
# tests/suites/spaces_ec2/upgrade_charm_with_bind.sh
# tests/suites/upgrade/streams.sh
# upgrades/operations.go
# upgrades/upgrade_test.go
# version/version.go
```

Includes PRs:
- #14424
- #14428
- #14430
- #14431
- #14427
- #14433
- #14434
- #14437
- #14439
- #14440
- #14442
- #14445
- #14446
- #14444
- #14448
- #14453
- #14436
- #14459
- #14462
- #14458
- #14418
- #14419
- #14463
- #14464
- #14455
- #14449
- #14465
- #14470
- #14473
- #14476
- #14468
- #14383
- #14483
This was referenced Aug 23, 2022
Merged
Merged
jujubot added a commit that referenced this pull request Sep 6, 2022
@jujubot
Merge pull request #14506 from basebandit/JUJU-1674_remove_nw_cloud_d…
925805b 
…isplay_python_test

#14506

This PR removes the old [cloud_display](https://github.com/juju/juju/blob/2.9/acceptancetests/assess_cloud_display.py) python tests which are now replaced by the new bash based counterpart [here](#14455)

## Checklist

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- ~[ ] Go unit tests, with comments saying what you're testing~
- [x] [Integration tests](https://github.com/juju/juju/tree/develop/tests), with comments saying what you're testing
- ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps

Not needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anvial anvial anvial approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@basebandit @anvial @jack-w-shaw @jujubot