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

[cloud-gce] Move integration tests to unit tests #12786

Merged
merged 2 commits into from
Aug 12, 2015
Merged

[cloud-gce] Move integration tests to unit tests #12786

merged 2 commits into from
Aug 12, 2015

Conversation

dadoonet
Copy link
Member

Basically this PR:

  • Update GCE API to latest version (1.71)
  • Replace IT by unit tests
  • Use Google Transport Mock to simulate JSON Response which helps to really test the implementation classes. That was not the case previously.
  • Remove GceSimpleITest as we now have RestIT (same goal for boths)

Would love to get feedback from reviewers. Thanks!

@dadoonet dadoonet added >test Issues or PRs that are addressing/adding tests review labels Aug 10, 2015
nik9000
nik9000 reviewed Aug 10, 2015
View reviewed changes
plugins/cloud-gce/src/main/java/org/elasticsearch/cloud/gce/GceComputeServiceImpl.java
@Override
public List<Instance> apply(String zoneId) {
try {
Compute.Instances.List list = client().instances().list(project, zoneId);
InstanceList instanceList = list.execute();
if (instanceList.isEmpty()) {
return new ArrayList();
return Lists.newArrayList();
Copy link
Member

Choose a reason for hiding this comment

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

I just saw @s1monw replacing Lists.newArrayList with new ArrayList<>() a few days ago. Maybe that way is out preferred way now?

+1 on removing the warning though.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah please just return Collections.EMPTY_LIST here or so?

@dadoonet
Copy link
Member Author

Tests are failing with:

Started J0 PID([email protected]).
Suite: org.elasticsearch.discovery.gce.GceDiscoveryTest
  2> REPRODUCE WITH: mvn test -Pdev -Dtests.seed=31458D3312839D7E -Dtests.class=org.elasticsearch.discovery.gce.GceDiscoveryTest -Des.logger.level=ERROR -Dtests.assertion.disabled=false -Dtests.security.manager=true -Dtests.heap.size=512m -Dtests.locale=fr_FR -Dtests.timezone=Europe/Paris
  2> NOTE: test params are: codec=Asserting(Lucene50): {}, docValues:{}, sim=RandomSimilarityProvider(queryNorm=false,coord=crazy): {}, locale=vi_VN, timezone=Australia/Sydney
  2> NOTE: Mac OS X 10.10.4 x86_64/Oracle Corporation 1.7.0_60 (64-bit)/cpus=4,threads=1,free=463145336,total=515375104
  2> NOTE: All tests run in this JVM: [GceDiscoveryTest]
ERROR   0.00s | GceDiscoveryTest (suite) <<<
   > Throwable #1: java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "accessClassInPackage.sun.misc")
   >    at __randomizedtesting.SeedInfo.seed([31458D3312839D7E]:0)
   >    at java.security.AccessControlContext.checkPermission(AccessControlContext.java:372)
   >    at java.security.AccessController.checkPermission(AccessController.java:559)
   >    at java.lang.SecurityManager.checkPermission(SecurityManager.java:549)
   >    at java.lang.SecurityManager.checkPackageAccess(SecurityManager.java:1525)
   >    at java.lang.Class.checkPackageAccess(Class.java:2304)
   >    at java.lang.Class.checkMemberAccess(Class.java:2284)
   >    at java.lang.Class.getDeclaredFields(Class.java:1805)
   >    at java.lang.Thread.run(Thread.java:745)
Completed [1/2] in 1.78s, 9 tests, 1 error <<< FAILURES!

Not sure what I should look at from this? Any idea @rjernst @rmuir ?

nik9000
nik9000 reviewed Aug 10, 2015
View reviewed changes
plugins/cloud-gce/src/main/java/org/elasticsearch/cloud/gce/GceComputeServiceImpl.java
.setApplicationName(Fields.VERSION)
.setHttpRequestInitializer(credential)
.build();
} catch (Exception e) {
logger.warn("unable to start GCE discovery service: {} : {}", e.getClass().getName(), e.getMessage());
logger.warn("unable to start GCE discovery service: {} : {}", e, e.getClass().getName(), e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

This looks not quite right. I'm not 100% sure, but I'd do:

+            logger.warn("unable to start GCE discovery service", e);

and let the logger do its job with the exception. I'm tired though - airplanes. So don't take what I type at face value.

@dadoonet
Copy link
Member Author

Thanks to @rmuir comment, I found my issue (and learned from that BTW) and fixed it. It's now running fine. Also fixed comments left in the first review.

Let me know!

I consider this PR as a first try on GCE. I'd like to do next the same cleanup on azure and aws if possible.

@clintongormley clintongormley mentioned this pull request Aug 12, 2015
Closed
16 tasks
@dadoonet dadoonet mentioned this pull request Aug 12, 2015
Closed
@dadoonet
Copy link
Member Author

I marked it as a blocker as we have a AwaitsFix in it.

s1monw
s1monw reviewed Aug 12, 2015
View reviewed changes
plugins/cloud-gce/src/main/java/org/elasticsearch/cloud/gce/GceComputeServiceImpl.java
super(settings);
this.project = settings.get(Fields.PROJECT);
String[] zoneList = settings.getAsArray(Fields.ZONE);
this.zoneList = Lists.newArrayList(zoneList);
this.zones = Lists.newArrayList(zoneList);
Copy link
Contributor

Choose a reason for hiding this comment

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

please use Arrays.asList while we re here

@s1monw
Copy link
Contributor

s1monw commented Aug 12, 2015

left some comments - this looks awesome

@dadoonet
Copy link
Member Author

@s1monw Updated with your comments.

@s1monw
Copy link
Contributor

s1monw commented Aug 12, 2015

left one comment LGTM otherwise

@dadoonet dadoonet force-pushed the gce/fix-test branch 2 times, most recently from f35145d to 7fa674e Compare August 12, 2015 15:42
@dadoonet
[cloud-gce] replace integration tests by unit tests
beb7e16 
We use google transport mock as we can simulate whatever JSON answer GCE platform will send us and really test Gce implementation.

We also remove GceSimpleITest as the goal of this class was only to check that when we start elasticsearch with this plugin, elasticsearch works fine.
We don't need that anymore as we now have RestIT which do that right (and better)!

Closes elastic#12622
@dadoonet dadoonet merged commit beb7e16 into elastic:master Aug 12, 2015
@dadoonet dadoonet removed the review label Aug 12, 2015
@dadoonet dadoonet deleted the gce/fix-test branch August 12, 2015 16:12
@clintongormley clintongormley added the :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs label Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
blocker :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >test Issues or PRs that are addressing/adding tests v2.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dadoonet @s1monw @nik9000 @rmuir @clintongormley