Reorganize routing-release test scripts #334

winkingturtle-vmw · 2023-08-16T15:28:13Z

Since most tests in this release required Linux, we now only support running tests in Docker and Concourse. This reorganization takes advantage of using the same task for running tests locally in docker and remotely in concourse.
There is a new requirement for running tests. That is wg-app-platform-runtime-ci that will host helper methods for configure_db and congiure_rsyslog and task for running tests under docker.
Add missing test-coverage for routing-info

Dependent PR:

https://github.com/cloudfoundry/wg-app-platform-runtime-ci/pull/7/files

Related PR:

What is this change about?

In your own words, describe this proposed change and the problem it solves. If GitHub has prepopulated any text above here with info from your commit message, please clean it up and account for it in this section.

What type of change is this?

[Breaking Change]: the change removes a feature or introduces a behavior change to core functionality (request routing, request logging)
[Minor Feature/Improvement]: the change introduces a new feature or improvement that doesn't alter core behavior
[Bug Fix]: the change addresses a defect

If you have selected multiple of the above, it may be wise to consider splitting this PR into multiple PRs to decrease the time for minor changes + bugs to be resolved.

Backwards Compatibility

If this is a breaking change, or modifies currently expected behaviors of core functionality (request routing or request logging), how has the change been mitigated to be backwards compatible?

Should this feature be considered experimental for a period of time, and allow operators to opt-in? Should this apply immediately to all routing-release deployments?

How should this be tested?

Are there any non-automated tests that should be performed against this change for validation? Please provide steps, and expected results for before + after the change.

Additional Context

Please provide any additional links or context (issues, other PRs, Slack discussions) to help understand this change.

PR Checklist

I have viewed, signed, and submitted the Contributor License Agreement.
I have made this pull request to the develop branch.
I have run all the unit tests using scripts/run-unit-tests-in-docker.
I have given thought to the backward-compatibility requirements of this change, and listed any mitigations above.
(Optional) I have run Routing Acceptance Tests and Routing Smoke Tests.
(Optional) I have run CF Acceptance Tests.

ebroberson

LGTM

linux-foundation-easycla · 2023-08-23T16:38:22Z

The committers listed above are authorized under a signed CLA.

✅ login: winkingturtle-vmw / name: Amin Jamali (0ee90dc)
✅ login: ebroberson / name: Brandon Roberson (9996c7b, 24f0a0c)

geofffranks · 2023-09-05T14:31:47Z

README.md

-  ```bash
-  ./scripts/run-unit-tests-in-docker gorouter
-  ```
+- `docker_run_routing_release_tests <mysql-8.0(or mysql),mysql-5.7,postgres>`:  This function combines steps described above sequentially.


Can we bring back a dockerized run-template-tests helper function/script?

I think ./scripts/template-tests is still part of the repo and can be executed inside/outside of the docker container.

geofffranks · 2023-09-05T14:37:18Z

scripts/docker/helpers.bash

+  local this_file_dir="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )"
+  "$this_file_dir/container.bash" "$db" -d
+  docker exec routing-release-docker-container '/repo/scripts/docker/build-binaries.bash'
+  docker exec routing-release-docker-container '/repo/scripts/docker/test.bash'


Is there ever a case where we'd want to run test.bash without first running build-binaries.bash - could these be merged together?

Yeah, not all tests require having the binaries in place. It's best to build-binaries once and have them in docker-container, but if one. wants to run tests for multi-error you don't have to build binaries. We did expose the test dependencies so that situation like this would not happen where routing-acceptance-tests was just relying on a really old version of rtr instead of forcing rtr to be built in case there was a problem with the code-base.

The idea here is to have a recipe for building-binaries per release. This will make it so that other releases don't have to figure out how to build rtr. The recipe would be pulled in from the CI for other release.

For context, diego's inigo builds gdn and gorouter and I remember fixing a bug with inigo a while back where it was still building gdn with runc instead of containerd.

geofffranks · 2023-09-05T14:41:21Z

scripts/docker/test.bash

+if [[ -n "${1:-}" ]]; then
+  test "src/code.cloudfoundry.org/${1}" "${2:-}"
+else
+  for component in gorouter cf-tcp-router multierror route-registrar routing-api routing-api-cli routing-info; do


Should this use this list of repos instead?

Or maybe instead just look at the contents of src/code.cloudfoundry.org/ and run test on each directory there?

Yes, that's a good catch, we should be pulling in the list from index.yml instead of the list here.

Started looking into fixing this today, but i'm not sure I like the idea of requiring the CI repo to be present in order to run local tests on routing-release.

We could have it default to auto-generating in routing-release if the CI repo isn't present to find the index.yml file, but then risk a false sense of security between local testing not matching CI testing.

Thinking about auditing, we would want to catch the following:

we tested fewer components than we expected

we tested more components than we expected

These would require two separate lists, and each would need to be updated as a component is added. I'm not sure if this is beneficial since if no action is done after adding a new component, it isn't tested, and nothing is brought to our attention. If it is added to one list but not the other, we get alerted to add it to another list. Most likely it would get added to the list of repos to test first, and then alerted to add to the audit list.

When using testing uses the same hardcoded list used for auditing, the audit becomes:

the list of components that were tested matches the codebases in the repo that we wanted to be tested

Like you said in the other PR's comments, it's difficult to programatically decide what components are important, since some are submodules from other releases, or different paths entirely (./src/code.cloudfoundry vs ./src).

It might be easier to update other releases to ensure everything needing to be tested is in src/code.cloudfoundry.org, and don't include components there if they shouldn't be tested (using a tools package to pull them in as vendored dependencies).

Did you have other methodologies for auditing what's being tested that I'm not coming up with?

geofffranks · 2023-09-05T14:49:13Z

scripts/docker/test.bash

+  local sub_package="${2:-}"
+
+  export DIR=${package}
+  . <(/ci/shared/helpers/extract-default-params-for-task.bash /ci/shared/tasks/run-bin-test/linux.yml)


Is this call needed since we're passing DEFAULT_PARAMS to the run-bin-test script on line 13?

This will load default values for params that are defined globally e.g. verify_go for VERIFICATIONS. The intention here is that the script that would run inside of the docker container would be exactly identical to what would get executed in concourse. In other words, by fetching the latest tas-runtime-postgres image and running the tests in docker we would see go versions mismatch between the release and docker container the same way as it would happen on concourse.

DEFAULT_PARAMS is convention by which a release e.g. routing-release would defines default values for params for which is either not defined or would like to be overridden. This is only needed for run-bin-test or build-binaries since those would be the most common thing a contributor would run.

Why is this path hardcoded to /ci/...?

- Since most tests in this release required Linux, we now only support running tests in Docker and Concourse. This reorganization takes advantage of using the same task for running tests locally in docker and remotely in concourse. - There is a new requirement for running tests. That is wg-app-platform-runtime-ci that will host helper methods for configure_db and congiure_rsyslog and task for running tests under docker. - Add missing test-coverage for routing-info - Remove ci directory in favor of wg-app-platform-runtime-ci repo Submodule src/code.cloudfoundry.org/cf-tcp-router 299348fc...5895502c: > Use bin/test.bash for running tests Submodule src/code.cloudfoundry.org/gorouter a17fb40b...95be16ee: > Update PR template > Use bin/test.bash for running tests Submodule src/code.cloudfoundry.org/multierror 52d6d177...ab6d83d7: > Use bin/test.bash for running tests Submodule src/code.cloudfoundry.org/route-registrar 98e6c81e...02ce3d1c: > Use bin/test.bash for running tests Submodule src/code.cloudfoundry.org/routing-acceptance-tests 883ec06a...f1f4c5cc: > Use bin/test.bash for running tests Submodule src/code.cloudfoundry.org/routing-api 332b19dd...d0e51c33: > Use bin/test.bash for running tests Submodule src/code.cloudfoundry.org/routing-api-cli e57e4fef...becd000e: > Use bin/test.bash for running tests Submodule src/code.cloudfoundry.org/routing-info ea57771d...8751f9f0: > Use bin/test.bash for running tests Signed-off-by: Brandon Roberson <broberson@vmware.com>

Signed-off-by: Geoff Franks <gfranks@vmware.com>

- Add REPO_NAME and REPO_PATH to .envrc so that scripts are more generic and could be used in other releases - Remove scripts/docker/helpers.bash in favor of ./scripts/test-in-docker-locally Signed-off-by: Amin Jamali <ajamali@vmware.com>

winkingturtle-vmw requested a review from a team as a code owner August 16, 2023 15:28

winkingturtle-vmw marked this pull request as draft August 16, 2023 15:28

ebroberson approved these changes Aug 16, 2023

View reviewed changes

winkingturtle-vmw force-pushed the with-bin/test.bash branch 6 times, most recently from 600017f to 70f6f43 Compare September 1, 2023 15:02

winkingturtle-vmw force-pushed the with-bin/test.bash branch from 70f6f43 to 294ad0a Compare September 1, 2023 15:56

winkingturtle-vmw marked this pull request as ready for review September 1, 2023 16:55

geofffranks reviewed Sep 5, 2023

View reviewed changes

geofffranks mentioned this pull request Sep 5, 2023

Add coverage for routing-release cloudfoundry/wg-app-platform-runtime-ci#7

Merged

Amin Jamali and others added 3 commits September 11, 2023 17:01

Add helper script for running local tests

9996c7b

Signed-off-by: Geoff Franks <gfranks@vmware.com>

winkingturtle-vmw force-pushed the with-bin/test.bash branch from 6db2bf7 to 24f0a0c Compare September 11, 2023 17:09

geofffranks approved these changes Sep 11, 2023

View reviewed changes

winkingturtle-vmw merged commit d823c96 into develop Sep 11, 2023

winkingturtle-vmw deleted the with-bin/test.bash branch September 11, 2023 18:54

winkingturtle-vmw mentioned this pull request Sep 11, 2023

bump cf-tcp-router #343

Merged

9 tasks

peanball mentioned this pull request Sep 25, 2023

Regression for local testing of Routing Release #352

Closed

ebroberson mentioned this pull request Dec 6, 2023

Request for Approver status cloudfoundry/community#727

Merged

Reorganize routing-release test scripts #334

Reorganize routing-release test scripts #334

Uh oh!

Conversation

winkingturtle-vmw commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this change about?

What type of change is this?

Backwards Compatibility

How should this be tested?

Additional Context

PR Checklist

Uh oh!

ebroberson left a comment

Choose a reason for hiding this comment

Uh oh!

linux-foundation-easycla bot commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geofffranks Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

winkingturtle-vmw commented Aug 16, 2023 •

edited

Loading

linux-foundation-easycla bot commented Aug 23, 2023 •

edited

Loading

geofffranks Sep 5, 2023 •

edited

Loading