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

chore: fix unit tests using gcloud ADC #3722

Merged
merged 90 commits into from
Apr 14, 2023

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Mar 22, 2023

Various changes to test suites, including a few adaptations necessary to either

  • correct tests, mainly by adapting to the new account switching mechanism
  • fix the component the failing test is evaluating

@diegomarquezp diegomarquezp changed the title chore: [wip] fix unit tests using gcloud ADC chore: fix unit tests using gcloud ADC Mar 31, 2023
@@ -39,34 +38,25 @@ public class CloudSdkProcessWrapperTest {

private final CloudSdkProcessWrapper wrapper = new CloudSdkProcessWrapper();

@Before
public void setUp() {
TestAccountProvider.setAsDefaultProvider(State.NOT_LOGGED_IN);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as a test suite does not instantiate a client (e.g. apiFactory.newXXXClient()), this account provider must be setup to prevent actual calls or credential checks

assertEquals(ex.getMessage(), "credential required for deploying");
}
}

@Test
public void testGetAppEngineDeployment_nonExistingCredentialFile()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Providing credentials does not apply anymore.

@@ -64,7 +65,9 @@ public void setUp() throws IOException {
Iam iam = mock(Iam.class);
Projects projects = mock(Projects.class);
ServiceAccounts serviceAccounts = mock(ServiceAccounts.class);


when(apiFactory.hasCredentialsSet()).thenReturn(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the api factory has to instantiate clients, we just follow the mockito approach.

@@ -136,7 +135,7 @@ public void testDefaults_values() {

assertTrue(component.isUseDefaultOptions());
Map<String, String> values = component.getValues();
assertEquals("pref-email", values.get(DataflowPreferences.ACCOUNT_EMAIL_PROPERTY));
assertEquals(TestAccountProvider.EMAIL_ACCOUNT_1, values.get(DataflowPreferences.ACCOUNT_EMAIL_PROPERTY));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dataflow preferences will not be able to set an email anymore. Only the one from the gcloud CLI will be picked up

public Map<String, String> getValues() {
Map<String, String> values = new HashMap<>();
values.put(DataflowPreferences.ACCOUNT_EMAIL_PROPERTY, defaultOptions.getAccountEmail());
values.put(DataflowPreferences.ACCOUNT_EMAIL_PROPERTY, getCurrentEmail());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where it defaults to the ADC email regardless if using custom preferences or default ones

@@ -81,6 +81,23 @@
* Assumed to be executed solely within the SWT UI thread.
*/
public class RunOptionsDefaultsComponent {

public enum ValidationStatus {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these both for debugging and better testability.

return account.isPresent() ? account.get().getEmail() : "";
}

private Optional<Account> getSelectedAccount() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added logic to fireSelectionListeners(). If the user logs in with a different account in gcloud, then this will trigger an account "selection", saving us from refactoring the account change logic in dependent components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is also the central point that fires the events. All other methods in this class, if they ever require account info, will rely on this method to ensure the event is fired

checkDependencyDirectives(attributeName, prefixToCheck, versionString, Lists.newArrayList());
}

private void checkDependencyDirectives(
Copy link
Contributor Author

@diegomarquezp diegomarquezp Mar 31, 2023

Choose a reason for hiding this comment

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

This test is inherited by other packages. It basically confirms that dependency with prefix X has version[ range] Y. These are hardcoded in this same class.
If we follow the suggestion in the non-cached credentials PR, then this change may have to be reverted as it would be innecessary.

@diegomarquezp diegomarquezp marked this pull request as ready for review March 31, 2023 21:54
@@ -116,7 +115,7 @@ public void testDefaults_nulls() {
assertTrue(component.isUseDefaultOptions());
Map<String, String> values = component.getValues();
// all empty/null settings should be turned to empty strings
Copy link
Member

Choose a reason for hiding this comment

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

Comment doesn't fit with the line below it anymore. Should testDefaults_nulls be providing an email?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

values.get(DataflowPreferences.ACCOUNT_EMAIL_PROPERTY) would return the email associated with ADC regardless of the pipeline options being default or not. I will modify this comment to indicate it

@@ -158,7 +157,7 @@ public void testCustomValues_nulls() {

// the empty/null customValues should override the preferences
Copy link
Member

@burkedavison burkedavison Apr 4, 2023

Choose a reason for hiding this comment

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

Comment doesn't fit with the email line below it anymore. Should testCustomValues_nulls be providing an email?

component.setUseDefaultValues(false);
assertTrue(!component.isUseDefaultOptions());
Copy link
Member

Choose a reason for hiding this comment

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

assertFalse(x) rather than assertTrue(!x) will provide a better error message if it fails.

}

} catch (IOException ex) {
fail("Unexpected IOException when setting up mocks");
Copy link
Member

Choose a reason for hiding this comment

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

Consider just throwing the exception, so when it happens the log will show what happened and the associated stack trace. It'll still fail the test.

import com.google.cloud.tools.eclipse.test.util.ui.CompositeUtil;
import com.google.cloud.tools.eclipse.test.util.ui.ShellTestResource;
import com.google.cloud.tools.login.Account;
import com.google.common.base.Optional;
Copy link
Member

Choose a reason for hiding this comment

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

From the Guava Optional javadocs:

However, we do gently recommend that you prefer the new, standard Java class whenever possible.

Consider using java.util.Optional instead.

try {
List<Project> projects = apiFactory.newProjectsApi().list().execute().getProjects();
for (String projectId : projectIds) {
assertTrue(projects.stream().allMatch(project -> project.getProjectId() == projectId));
Copy link
Member

Choose a reason for hiding this comment

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

Given a list of project IDs, get all projects from the API factory and ensure that each of those projects have all the given project IDs.

This seems like it only works with singleton lists, since a project is only going to have one ID (I would hope). Was the intent anyMatch? If so, consider using Truth's fuzzy matcher for a better error message. Something like...

Truth.assertThat(list).containsExactlyElementsIn(
    otherList.stream().map(Project::getProjectId().collect(toList())
);

Copy link
Contributor Author

@diegomarquezp diegomarquezp Apr 11, 2023

Choose a reason for hiding this comment

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

The intent was to confirm an account was switched and a list with certain projects was loaded for such user but the test setup uses a list with a single project for both test users. I'll try the fuzzy matcher I think there is no Truth in the project - anyMatch makes sense too

assertTrue(projects.stream().allMatch(project -> project.getProjectId() == projectId));
}
} catch (IOException ex) {
fail();
Copy link
Member

Choose a reason for hiding this comment

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

Consider throwing ex, or this is going to be hard to debug from just the logs.


@VisibleForTesting
boolean getCanEnableChildren() {
return canEnableChildren;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what google style recommends, but consider dropping the get:

boolean canEnableChildren() { return canEnableChildren; }

try {
account = Optional.of(apiFactory.getAccount());
} catch (IOException ex) {
// will return default
Copy link
Member

Choose a reason for hiding this comment

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

The fact that we have to keep catching this generic IOException when calling getAccount() is a bit concerning due to the possibility that the next maintainer isn't going to realize this is the contract that's required -- and it's hard for me as a reviewer to ensure that every invocation of getAccount has appropriate error catching.

* @return 1 if logged in, 0 if not
*/
public int getAccountCount() {
// TODO Auto-generated method stub
Copy link
Member

Choose a reason for hiding this comment

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

TODO


private void fireSelectionListeners() {
for (Object o : selectionListeners.getListeners()) {
((Runnable) o).run();
Copy link
Member

Choose a reason for hiding this comment

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

Are these truly all guaranteed to be Runnables into the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is guaranteed, as Runnables can be added/removed only by methods accepting Runnables.
This piece of code is technically untouched when compared to the main branch (same for adding/removing listeners).

The documentation of Eclipse's ListenerList (the container used here) suggests treating the list as Object[].

LOGGED_IN_SECOND_ACCOUNT
}

private static Map<State, Account> accounts = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

EnumMap is a nice choice for this situation, but not required.

#3726)

* chore: change credential and account to `Optional`

* chore: adapt localserver tests

* chore: adapt dataflow plugin

* chore: adapt login plugin

* chore: adapt projectselector tests

* chore: adapt projectselector (credential)

* chore: adapt appengine deploy (hascredentials)

* chore: move ITestAccountProvider

* chore: adapt appengine.deploy (hascredentials)

* chore: adapt appengine.localserver (hascredentials)

* chore: adapt dataflow.ui (hascredentials)

* chore: adapt googleapis test (hascredential)

* chore: adapt login (hascredentials)

* chore: adapt projectselector (hascredentials)

* chore: fix setup in gcplocalruntabtest

* chore: adapt test account provider to new interface

* chore: remove `hasCredentialsSet` from `TestAccountProvider`

* chore: test fixes

* chore: correct import (`java.util.Optional`)

* chore: workaround for undetected mock in `FlexDeployPreferencesDialogTest`

* chore: remove unused import

* chore: `Optional` convenience methods
@diegomarquezp diegomarquezp merged commit c77e46b into gcloud-cli-creds Apr 14, 2023
@diegomarquezp diegomarquezp deleted the gcloud-cli-creds-test-fixes branch April 14, 2023 20:26
diegomarquezp added a commit that referenced this pull request Jun 6, 2023
* Adjustments to use application default credentials

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC (#3715)

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC

* fix: use `setBaseEnabled()` to correctly propagate login state

* chore: better UI, on-demand cred check

* chore: adapt single account to deploy dialog (#3717)

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC

* fix: use `setBaseEnabled()` to correctly propagate login state

* chore: better UI, on-demand cred check

* chore: [wip] adapt single account to deploy dialog

* chore: display gcloud instruction messages

* fix: layout to wrap no ADC message

* chore: cleanup of `gcloud-cli-creds` (#3714)

* chore: cleanup of deploy classes

* chore: cleanup of deploy UI classes

* chore: cleanup of cloudsdk staging helper

* chore: better missing-credentials handling

* chore: removal of Credential argument in `newApi()` methods (#3713)

* feat: initial removal of Credential argument in newApi() methods

* chore: move account resolution to new class

* chore: correct precond check, correct exception logging

* chore: do not throw exception when checking for credentials

* fix: compilation errors

* fix: compilation errors in tests

* chore: enable or disable deploy button depending on ADC being found. (#3718)

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC

* fix: use `setBaseEnabled()` to correctly propagate login state

* chore: better UI, on-demand cred check

* chore: [wip] adapt single account to deploy dialog

* chore: display gcloud instruction messages

* fix: layout to wrap no ADC message

* chore: disable deploy button when not logged in

* chore: use new `hasCredentialsSet` method

* chore: fix unit tests using gcloud ADC (#3722)

* feat: initial removal of Credential argument in newApi() methods

* chore: move account resolution to new class

* chore: cleanup of deploy classes

* chore: cleanup of deploy UI classes

* chore: cleanup of cloudsdk staging helper

* chore: correct precond check, correct exception logging

* feat: modify "Manage Google Accounts" button to rely on gcloud ADC

* fix: use `setBaseEnabled()` to correctly propagate login state

* chore: better UI, on-demand cred check

* chore: [wip] adapt single account to deploy dialog

* chore: display gcloud instruction messages

* fix: layout to wrap no ADC message

* chore: disable deploy button when not logged in

* chore: do not throw exception when checking for credentials

* fix: compilation errors

* fix: compilation errors in tests

* chore: initial compilation-enabling adjustments

* chore: use `hasCredentialsSet()`

* chore: add test account provider

* chore: fix usagetracker and projectselector test

* chore: expose `getCredential()`

* chore: fix projectselector tests

* chore: null check for `setPRoviderState()`

* chore: add avatar URL to test accounts

* chore: fix login tests

* chore: fix googleapis tests

* chore: add convenience state+defaultprovider method

* chore: fix dataflow tests

* chore: fix dataflow core tests

* chore: fix error-prone state check in localserver

* chore: correct static member initialization order in `TestAccountProvider`

* chore: initialize static account provider once

* chore: use mock methods instead of test acct provider in `MiniSelectorTest`

* chore: fixes after merging `gcloud-cli-creds`

* chore: use avatar urls in test accounts

* chore: fix accounts panel tests

* chore: fix dataflow preferences test

* chore: fix login `Account` package

* chore: fix usagetracker test ii

* fix: project repository test

* chore: fix googleapis proxy tests

* fix: dataflow preferences test

* chore: fix login tests, remove unused elements (pending cleanup)

* chore: fix app engine deploy tests

* chore: remove unnecessary test in `GcpProjectQueryJobTest`

* chore: fix localserver tests iii

* chore: more dataflow ui test fixes

* chore: fix project repository test

* chore: accounts panel test to assert correct number of children

* chore: run tests on any pr

* chore: fix update accounts logic in `GoogleLoginService`

* chore: remove initialization of static members in constructors

* chore: fire listeners in `AccountSelector`

* chore: fix `GoogleApiFactory` initalization in `GcpLocalRunTabTest`

* chore: fix cache check in `AccountSelector`

* chore: fix `CloudSdkProcessWrapperTest`

* chore: correct stubbing in `DefaultedPipelineOptionsComponentTest`

* chore: remove unused imports in `CloudSdkProcessWrapper`

* chore: remove unused import in `AccountPanelsTest`

* chore: trigger update on `GcpLocalRunTabTest`

* chore: fix `fireSelectionListeners` after setting change tracker variable

* chore: fix `GcpLocalRUnTabTest`

* chore: refactor force `AccountSelector` check to a better named method

* chore: return validation states in `RunOptionsDefaultsComponent`

* chore: add status checks to other test in `RODCTest`

* chore: make `forceCheck()` public in `AccountSelector`

* chore: remove unused thrown exception from delcaration in `GcpLocalRunTabTest`

* chore: set flag to enable `forceAccountCheck`

* chore: have dataflow Command RunOptionsDefaultScomponent not found to return gcloud CLI email

* chore: fix login service test

* chore: fix assertions, verify projects loaded in `RunOptionsDefaultCOmponentsStest`

* chore: update project lists on account change in `RunOptionsDefaultsComponent`

* chore: lock `login` `com.google.api` versions to `1.25.0`

* chore: remove unthrown exception declaration in `GcpLocalRunTabTest`

* chore: fix tests in `RunOptionsDefaultsCOmponentTest`

* chore: correct versions in login MANIFEST.MF

* chore: fix Command setUpServiceKeyCreation not found in Command GcpLocalRunTabTest not found'

* chore: make account selector to return update result

* chore: fix non-existent version of `com.google.api.services.oauth2`

* chore: fix logic check in `forceAccountCHeck` for `AccountSelector`

* chore: exclude `oauth2` package in manifest check

* chore: fix comments in default pipeline options test

* chore: fixes in dataflow tests

* chore: remove TODO comment

* chore: using `EnumMap` in `TestAccountProvider`

* chore: handle `GoogleApiFactory` credential-related methods internally (#3726)

* chore: change credential and account to `Optional`

* chore: adapt localserver tests

* chore: adapt dataflow plugin

* chore: adapt login plugin

* chore: adapt projectselector tests

* chore: adapt projectselector (credential)

* chore: adapt appengine deploy (hascredentials)

* chore: move ITestAccountProvider

* chore: adapt appengine.deploy (hascredentials)

* chore: adapt appengine.localserver (hascredentials)

* chore: adapt dataflow.ui (hascredentials)

* chore: adapt googleapis test (hascredential)

* chore: adapt login (hascredentials)

* chore: adapt projectselector (hascredentials)

* chore: fix setup in gcplocalruntabtest

* chore: adapt test account provider to new interface

* chore: remove `hasCredentialsSet` from `TestAccountProvider`

* chore: test fixes

* chore: correct import (`java.util.Optional`)

* chore: workaround for undetected mock in `FlexDeployPreferencesDialogTest`

* chore: remove unused import

* chore: `Optional` convenience methods

* chore: adapt tests of `AccountSelector` (#3728)

* chore: remove unnecessary login module classes (#3727)

* chore: remove login service OAuth related classes

* chore: adapt appengine deploy

* chore: adapt appengine deploy tests

* chore: adapt dataflow core

* chore: adapt dataflow core tests

* chore: adapt dataflow ui

* chore:adapt dataflow ui tests

* chore: adapt tests in login ui

* chore: fix test case in dataflow core

* chore: improve Optional usage

* chore: update appengine-plugins-core to 0.9.11 (#3732)

* chore: fix project list initialization in local server launch config (#3729)

* chore: fix project list initialization in local server launch config

* chore: adapt tests to initially set credentials

* chore: initialize account selector at right time

* chore: fix initialization

* chore: fix initialization ii

* chore: initialize account selector with empty creds

* chore: initialize api factory before instantiating the localserver tabl

* chore: (cleanup) convert `GoogleApiFactory` to static usage (#3733)

* chore: update snakeyaml to 2.0

* chore: adapt appengine.deploy.ui

* chore: adapt appengine.deploy.ui.test

* chore: remove usages in `appengine.deploy.ui`

* chore: remove reference from appengine.deploy.ui.test

* chore: hide constructor in googleapis

* chore: adapt localserver

* chore: adapt localserver tests

* chore: adapt dataflow core

* chore: adapt dataflow ui

* chore: adapt dataflow ui test

* chore: adapt googleapis

* chore: adapt login

* chore: adapt projectselector

* chore: adapt projectselector tests

* chore: delete `GoogleLoginService` (due at last merge)

* chore: compilation errors

* chore: compilation errors ii

* chore: adapt localserver test ii

* chore: adapt dataflow core test ii

* chore: adapt dataflow ui ii

* chore: adapt datafow ui test ii

* chore: use GoogleApiFactory mock

* chore: use static instance in defaulted pipeline options

* chore: more compilation errors

* feat: compute credentials from well-known ADC file path (apiary credentials) (#3731)

* chore: compute adc path with auth java library

* chore: add polling and subscription to adc

* chore: fix listener logic for credential changes

* chore: compute cred identifier (refresh token) manually

* chore: adapt test account provider

* chore: listen to cred changes in credentials menu item

* chore: non optional-wrapped ADC file

* chore: add dispose listener to account selector

* chore: use `WatchService`

* chore: tests for `DefaultAccountProvider`

* chore: update snakeyaml to 2.0

* chore temp folder to be public

* chore: add more propagation checks

* chore: reorder change counters

* chore: debug info for test

* chore: more logging

* chore: info level for adc watcher

* chore: use custom constructor

* chore: fix constructor

* chore: wait for adc file change in test

* chore: use class level listener

* chore: mock non-final method to identify credential

* chore: make `CredentialWithId` public

* chore: various test fixes

* test: fix listener wait workflow

* chore: adapt to static apifacotry instance

* chore: adapt to static apifactory instance ii

* chore: correct tests

* chore: compute custom refresh token

* chore: improve logging

* chore: more logging

* chore: increase delay between event handling

* chore: more logging ii

* chore: thread safety

* chore: changes polling logging

* chore: fix break on polling

* chore: thread safety in credentials

* chore: more logging for account and credential test logic

* chore: final test fixes

* chore: fix logging levels for production class

* chore: better path resolution

* chore: init transport cache manually

* chore: exclide OSGI-INF from build

* chore: adapt transport cache tests

* chore: correct reactivity in appengine deploy dialog (#3735)

* chore: remove `@Component` annotation from api factory

* chore: react on credential change via gcloud CLI

* chore: remove comments

* chore: handle correct exception type

* chore: grace period for json file to be written

* chore: use separate thread logic to check adc file

* chore: accomodate (deprecated) dataflow tests

* chore: accomodate (deprecate) dataflow tests ii

* chore: accomodate (deprecate) dataflow tests iii

* chore: revert default pipeline adjustments

* chore: reset mock singleton in `GoogleApiFactory`

* chore: remove unused import

---------

Co-authored-by: Burke Davison <burkedavison@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants