fix(gcb): Return mutable lists from methods annoated with PostFilter #673
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The GCB integration tests currently don't run the postFilter logic that is added as an annotation on controller methods. I believe this is because we've been to selective in exactly the beans that should be present in the test, and are not pulling in the required Spring Security beans. In order to fix this, add the same @componentscan that we have on Main.class to the test so it pulls in the same beans. (We can then also remove the specific controller bean we'd been pulling in, but will leave all the configuration beans in place.) This causes the listAccountTest to fail because the listAccounts function is currently broken; the next commit will fix the function and the test.
Controller methods annotated with PostFilter cannot return immutable lists because the filtering is done in-place on the returned array; update all controller methods in GoogleCloudBuildController to return a mutable list. As we would like the lower levels of the stack to deal in immutable collections as much as possible, just create a mutable collection at the last step before returning from the controller method.
|
Will look into that failing test, this is passing locally but not in Github Actions. |
As we are now fully loading the application context as part of the GCB tests, we should mark the test with DirtiesContext so that it does not interfere with other tests.
The tests were failing because we can only ever create a single HystrixSpectatorPublisher bean, due to the fact that it mutates a static field on the HystrixPlugins class, which causes a failure if we try to mutate it again. This was addressed in MainTest by clearing the Hystrix class after every test, but let's just inject a mock bean instead to avoid mutating the static class in the first place.
|
@spinnakerbot cherry-pick 1.19 |
spinnakerbot
pushed a commit
that referenced
this pull request
Mar 20, 2020
…673) * test(gcb): Update tests to run postFilter logic The GCB integration tests currently don't run the postFilter logic that is added as an annotation on controller methods. I believe this is because we've been to selective in exactly the beans that should be present in the test, and are not pulling in the required Spring Security beans. In order to fix this, add the same @componentscan that we have on Main.class to the test so it pulls in the same beans. (We can then also remove the specific controller bean we'd been pulling in, but will leave all the configuration beans in place.) This causes the listAccountTest to fail because the listAccounts function is currently broken; the next commit will fix the function and the test. * fix(gcb): Return mutable lists from methods annoated with PostFilter Controller methods annotated with PostFilter cannot return immutable lists because the filtering is done in-place on the returned array; update all controller methods in GoogleCloudBuildController to return a mutable list. As we would like the lower levels of the stack to deal in immutable collections as much as possible, just create a mutable collection at the last step before returning from the controller method. * fix(gcb): Dirty application context for GCB tests As we are now fully loading the application context as part of the GCB tests, we should mark the test with DirtiesContext so that it does not interfere with other tests. * fix(gcb): Use mock HystrixSpectatorPublisher bean The tests were failing because we can only ever create a single HystrixSpectatorPublisher bean, due to the fact that it mutates a static field on the HystrixPlugins class, which causes a failure if we try to mutate it again. This was addressed in MainTest by clearing the Hystrix class after every test, but let's just inject a mock bean instead to avoid mutating the static class in the first place.
|
Cherry pick successful: #674 |
ezimanyi
added a commit
that referenced
this pull request
Mar 20, 2020
…673) (#674) * test(gcb): Update tests to run postFilter logic The GCB integration tests currently don't run the postFilter logic that is added as an annotation on controller methods. I believe this is because we've been to selective in exactly the beans that should be present in the test, and are not pulling in the required Spring Security beans. In order to fix this, add the same @componentscan that we have on Main.class to the test so it pulls in the same beans. (We can then also remove the specific controller bean we'd been pulling in, but will leave all the configuration beans in place.) This causes the listAccountTest to fail because the listAccounts function is currently broken; the next commit will fix the function and the test. * fix(gcb): Return mutable lists from methods annoated with PostFilter Controller methods annotated with PostFilter cannot return immutable lists because the filtering is done in-place on the returned array; update all controller methods in GoogleCloudBuildController to return a mutable list. As we would like the lower levels of the stack to deal in immutable collections as much as possible, just create a mutable collection at the last step before returning from the controller method. * fix(gcb): Dirty application context for GCB tests As we are now fully loading the application context as part of the GCB tests, we should mark the test with DirtiesContext so that it does not interfere with other tests. * fix(gcb): Use mock HystrixSpectatorPublisher bean The tests were failing because we can only ever create a single HystrixSpectatorPublisher bean, due to the fact that it mutates a static field on the HystrixPlugins class, which causes a failure if we try to mutate it again. This was addressed in MainTest by clearing the Hystrix class after every test, but let's just inject a mock bean instead to avoid mutating the static class in the first place. Co-authored-by: Eric Zimanyi <ezimanyi@google.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes spinnaker/spinnaker#5593
test(gcb): Update tests to run postFilter logic
The GCB integration tests currently don't run the postFilter logic that is added as an annotation on controller methods. I believe this is because we've been to selective in exactly the beans that should be present in the test, and are not pulling in the required Spring Security beans.
In order to fix this, add the same @componentscan that we have on Main.class to the test so it pulls in the same beans. (We can then also remove the specific controller bean we'd been pulling in, but will leave all the configuration beans in place.)
This causes the listAccountTest to fail because the listAccounts function is currently broken; the next commit will fix the function and the test.
fix(gcb): Return mutable lists from methods annoated with PostFilter
Controller methods annotated with PostFilter cannot return immutable lists because the filtering is done in-place on the returned array; update all controller methods in GoogleCloudBuildController to return a mutable list.
As we would like the lower levels of the stack to deal in immutable collections as much as possible, just create a mutable collection at the last step before returning from the controller method.