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.

Sign up for GitHub

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

Jump to bottom

fix: sets the project for the integration tests #386

Merged
merged 2 commits into from Aug 17, 2020
Merged

fix: sets the project for the integration tests #386

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c6d9f1
fix: sets the project for the integration tests
thiagotnunes Aug 17, 2020
ba177b7
fix: updates instructions for it tests
thiagotnunes Aug 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion google-cloud-spanner/pom.xml
View file
Open in desktop
Expand Up @@ -69,6 +69,7 @@
<systemPropertyVariables>
<spanner.testenv.config.class>com.google.cloud.spanner.GceTestEnvConfig</spanner.testenv.config.class>
<spanner.testenv.instance>projects/gcloud-devel/instances/spanner-testing</spanner.testenv.instance>
<spanner.gce.config.project_id>gcloud-devel</spanner.gce.config.project_id>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this break running the integration tests locally if you don't have permission to access the gcloud-devel project, unless you specify a different value for this system property in your maven command? Currently the integration tests use the Google Cloud project that is specified in your environment in combination with the credentials that have also been configured in your environment.

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 thought so, but the test instance is already hardcoded to projects/gcloud-devel/instances/spanner-testing (see property spanner.testenv.instance). I think that the only way you can run the IT tests currently is by overriding the two properties. Maybe we should update the CONTRIBUTING.md file here as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, you're right. My local config overrides the spanner.testenv.instance property. This change would mean that I would also need to override the spanner.gce.config.project_id property, but that is not really a big ask when the first one needs to be overridden anyways. As you noted, this might be worth noting in the CONTRIBUTING.md.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated the CONTRIBUTING.md with the new integration tests instructions.

</systemPropertyVariables>
<forkedProcessTimeoutInSeconds>3000</forkedProcessTimeoutInSeconds>
</configuration>
Expand Down Expand Up @@ -363,4 +364,4 @@
</build>
</profile>
</profiles>
</project>
</project>
3 changes: 1 addition & 2 deletions google-cloud-spanner/src/test/java/com/google/cloud/spanner/IntegrationTestEnv.java
View file
Open in desktop
Expand Up @@ -16,7 +16,6 @@

package com.google.cloud.spanner;

import static com.google.cloud.spanner.testing.EmulatorSpannerHelper.isUsingEmulator;
import static com.google.common.base.Preconditions.checkState;

import com.google.api.gax.longrunning.OperationFuture;
Expand Down Expand Up @@ -80,7 +79,7 @@ protected void before() throws Throwable {
SpannerOptions options = config.spannerOptions();
String instanceProperty = System.getProperty(TEST_INSTANCE_PROPERTY, "");
InstanceId instanceId;
if (!instanceProperty.isEmpty() && !isUsingEmulator()) {
if (!instanceProperty.isEmpty()) {
instanceId = InstanceId.of(instanceProperty);
isOwnedInstance = false;
logger.log(Level.INFO, "Using existing test instance: {0}", instanceId);
Expand Down