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.

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

Add tests for user-provided services #484

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

andy-paine
Copy link
Contributor

What is this change about?

Add tests for user-provided service operations

Please provide contextual information.

As part of fixing cloudfoundry/cloud_controller_ng#2543 it is useful to have some CATs that exercise the user-provided service behaviour. In particular the test that service bindings get updated when the service gets updated will currently fail on cf8 due to the CC v3 endpoint not having the correct behaviour.

Given these are dedicated user-facing commands on the CLI (cf cups and cf uups), I figured it made sense to have some CATs to cover their behaviour

What version of cf-deployment have you run this cf-acceptance-test change against?

Latest (v16.25.0)

Please check all that apply for this PR:

  • introduces a new test --- Are you sure everyone should be running this test? Yes, they are standard behaviour and they are fast
  • changes an existing test
  • requires an update to a CATs integration-config - this felt like a new set of tests but I could understand if you would prefer them to fall under the existing service tests flag

Did you update the README as appropriate for this change?

  • YES
  • N/A

How should this change be described in cf-acceptance-tests release notes?

Add tests for user-provided service operations

How many more (or fewer) seconds of runtime will this change introduce to CATs?

~60s with 4 nodes (slowest test is 60 seconds, fastest are <2 seconds)

What is the level of urgency for publishing this change?

  • Urgent - unblocks current or future work
  • Slightly Less than Urgent

Tag your pair, your PM, and/or team!

Working with team at SAP (can contact @philippthun, @FloThinksPi or @stephanme for followup)

Copy link

@JenGoldstrich JenGoldstrich left a comment

Choose a reason for hiding this comment

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

Hey Andy!

Thanks for submitting this PR, @ctlong and I just reviewed it, and also ran it against a 16.25 cf-deployment, all of the tests pass but one relating to the CAPI bug you mention (using uups to change the credentials parameter) Carson and I manually reproduced this bug on newest cf-deployment, I assume we just need to wait for a capi-release with this PR cloudfoundry/cloud_controller_ng#2558?

Three other small things outside of the things we marked in review that we would ask you to change

  • Adding include_user_provided_services to the example cats config
  • Adding a row to the Explanation of Test Groups section of the readme for user provide services
  • Adding unit tests to the CATS config unit test to validate the default is set correctly, nil error handling, and setting the value.

Fundamentally makes sense to us though, and the breaking test seems like it should pass when the relevant bug is fixed on CAPI, we should be able to re-test this on an appropriate CF-Deployment and merge after that.

Thanks
Jenna and Carson

user_provided_services/lifecycle.go Outdated Show resolved Hide resolved
user_provided_services/lifecycle.go Show resolved Hide resolved
This includes a test that updating a service updates bindings which will
only work for cf CLI v6 or v7. The v8 CLI uses the v3 endpoint to update
service instances which is currently broken (see
cloudfoundry/cloud_controller_ng#2543)

Signed-off-by: Andy Paine <andrew.paine@sap.com>
@andy-paine
Copy link
Contributor Author

Thanks @JenGoldstrich - that's really useful feedback. I think it is best to wait for a CAPI release to get these merged - in theory the cf-deployment pipelines are still using cf7 which uses the v2 services endpoints so these tests actually pass (I'm assuming you run cf8 locally which is where you saw the failures). These tests should pass in all scenarios though rather than just the cf-d pipelines so let's hold off until all CLI versions work 👍

@andy-paine
Copy link
Contributor Author

Can we merge this now that CAPI 1.22.0 is on the develop branch of cf-deployment or do we need to wait for a full cf-deployment release?

@ctlong
Copy link
Member

ctlong commented Dec 10, 2021

Thanks @andy-paine, looks great!

@ctlong ctlong merged commit 684f995 into cloudfoundry:develop Dec 10, 2021
CF Acceptance Tests automation moved this from Review in progress to Done Dec 10, 2021
@FloThinksPi FloThinksPi deleted the add-ups-tests branch December 13, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants