Skip to content

Conversation

@bojeil-google
Copy link
Contributor

Adds integration tests for tenant management APIs.
These tests are skipped by default as multi-tenancy is a paid feature on Google Cloud Identity Platform.
To run these tests, --testMultiTenancy flag has to be added.
Adds d.ts references for multi-tenancy APIs.
Adds default email provider config when backend Auth server returns undefined for emailSignInConfig.

These tests are skipped by default as multi-tenancy is a paid feature on Google Cloud Identity Platform.
To run these tests, --testMultiTenancy flag has to be added.
Adds d.ts references for multi-tenancy APIs.
Adds default email provider config when backend Auth server returns undefined for emailSignInConfig.
Copy link

@wuyanna wuyanna left a comment

Choose a reason for hiding this comment

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

Just curious, do these tests call backend production endpoints? How often do they run? And which test project does it use?

@bojeil-google
Copy link
Contributor Author

These tests are calling backend production endpoints. They are currently only run manually before a new binary release of the library is pushed. It is basically a sanity check for our new releases (they act more like e2e tests). They are not specific to one project.

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

This looks pretty good. Just a couple of suggestions.

// Test listTenant pagination.
return admin.auth().listTenants(1);
})
.then((result) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Something we do in other SDKs to test list operations is to list everything, and see if the known results appear in the output. Something like:

const allTenantIds = listAndIterateThroughAllTenants();
// scan allTenantIds to see if createdTenants is a subset of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/index.d.ts Outdated
* resets, password or email updates, etc).
*/
tokensValidAfterTime?: string;

Copy link
Contributor

Choose a reason for hiding this comment

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

These require a review from @egilmorez or somebody from his team. How about we make these changes in a separate PR, so the rest of the code changes don't get blocked on documentation updates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted the reference changes. Will have a follow up PR just for that.

Reverts index.d.ts reference changes. These will be added in a separate PR.
Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM with a nit.

src/index.d.ts Outdated
}

export = admin;
export = admin;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@bojeil-google bojeil-google merged commit 99eef4a into auth-multi-tenancy Jun 18, 2019
@bojeil-google bojeil-google deleted the temp-multi-tenancy branch June 25, 2019 07:15
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.

3 participants