Upgrade Pub/Sub samples. by jmdobry · Pull Request #216 · GoogleCloudPlatform/nodejs-docs-samples

jmdobry · 2016-09-26T17:39:42Z

Switch to region tags that match other languages' samples
Simplify tests (cut amount of test code in half)
Achieved 100% code coverage (as opposed to 95.5% or whatever we've been limited to previously
Added sample for creating a Push Subscription
Begin to use ES2015 features available in the Node.js 4.5.0 LTS release

Compare to:

codecov-io · 2016-09-26T18:15:32Z

Current coverage is 90.28% (diff: 100%)

Merging #216 into master will increase coverage by 72.91%

@@             master       #216   diff @@
==========================================
  Files            33         54     +21   
  Lines          1917       2347    +430   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
+ Hits            333       2119   +1786   
+ Misses         1584        228   -1356   
  Partials          0          0

Powered by Codecov. Last update ecef164...4d8e931

ace-n

Mostly LGTM (other than my comments, and assuming all tests pass).

ace-n · 2016-09-27T17:35:15Z

pubsub/subscriptions.js

-  // See https://googlecloudplatform.github.io/google-cloud-node/#/docs/pubsub/latest/pubsub/subscription?method=delete
-  subscription.delete(function (err, apiResponse) {
+  // Lists all subscriptions for the topic
+  topic.getSubscriptions((err, subscriptions) => {


I'm assuming that this function-calling style is sufficiently idiomatic.

ace-n · 2016-09-27T17:36:26Z

pubsub/subscriptions.js


-    console.log('Got metadata for subscription: %s', subscriptionName);
-    return callback(null, metadata);
+    console.log(`Subscription ${subscription.name} created.`);


(Same assumption about idiomatic-ness here.)

ace-n · 2016-09-27T17:39:30Z

pubsub/subscriptions.js

-  console.log('received message: ' + message.data);
+    console.log(`Subscription: ${metadata.name}`);
+    console.log(`Topic: ${metadata.topic}`);
+    console.log(`Push config: %s`, metadata.pushConfig.pushEndpoint);


(Nit - I'm assuming we care): is there a way to make this line consistent with the others?

ace-n · 2016-09-27T17:40:20Z

pubsub/subscriptions.js

-    var ackIds = messages.map(function (message) {
-      return message.ackId;
+    messages.forEach((message) => {
+      console.log(`* ${message.id} %j %j`, message.data, message.attributes);


Same nit as above - do we care about consistency in how arguments to console.log are formatted?

Hmm, I'll pull message.id out, because pulling the other two in doesn't work so well.

ace-n · 2016-09-27T18:04:29Z

pubsub/system-test/subscriptions.test.js

-          done();
-        });
+  after((done) => {
+    pubsub.subscription(subscriptionNameOne).delete(() => {


Should we be deleting subscriptionNameTwo as well?

ace-n · 2016-09-27T18:05:57Z

pubsub/system-test/subscriptions.test.js

+      const output = run(`${cmd} list`, cwd);
+      assert.notEqual(output.indexOf(`Subscriptions:`), -1);
+      assert.notEqual(output.indexOf(fullSubscriptionNameOne), -1);
+      done();


Does fullSubscriptionNameTwo show up here - and if it does, should we check for it?

ace-n · 2016-09-27T18:06:18Z

pubsub/system-test/subscriptions.test.js

+    // Listing is eventually consistent. Give the indexes time to update.
+    const output = run(`${cmd} list ${topicName}`, cwd);
+    assert.notEqual(output.indexOf(`Subscriptions for ${topicName}:`), -1);
+    assert.notEqual(output.indexOf(fullSubscriptionNameOne), -1);


Ditto to above comment (fullSubscriptionNameTwo)

ace-n · 2016-09-27T18:12:19Z

pubsub/system-test/topics.test.js

-        assert(console.log.calledWith('Published %d message(s)!', messageIds.length));
-        assert.notEqual(apiResponse, undefined);
+        console.log(JSON.stringify(messages, null, 2));
+        assert.equal(messages[0].data, message.data);


I'm assuming the newest message always comes first, and that we don't have to worry about message order?

Pub/Sub messages are unordered (they can come in any order).

In this test environment, we know there's only one message because we just created the topic and subscription and published a single message.

ace-n · 2016-09-27T18:12:43Z

pubsub/system-test/topics.test.js

-        assert(console.log.calledWith('Deleted topic: %s', topicName));
-        assert.notEqual(apiResponse, undefined);
+        console.log(JSON.stringify(messages, null, 2));
+        assert.deepEqual(messages[0].data, message);


Ditto assumption about message order.

See #216 (comment)

jmdobry · 2016-09-27T18:54:02Z

Regarding message ordering, I've actually got a new Pub/Sub sample in another branch that shows how to manually enforce ordering with a stateful counter.

ace-n · 2016-09-27T20:49:10Z

LGTM, assuming that datastore test is better off removed than skipped. 👍

jmdobry · 2016-09-27T22:22:20Z

I just removed the test because you removed the method it was testing in a previous PR. I'm not sure how we missed the test...

…ds (#216) Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [X] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/nodejs-game-servers/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [X] Ensure the tests and linter pass - [X] Code coverage does not decrease (if any source code was changed) - [X] Appropriate docs were updated (if necessary) Fixes b:179738605 🦕

docs(samples): Adds start of region tag

* updated CHANGELOG.md * updated package.json * updated samples/package.json

jmdobry force-pushed the pubsub-sample-style branch from b3edcf9 to e5ced9e Compare September 26, 2016 17:40

jmdobry mentioned this pull request Sep 26, 2016

[DO NOT MERGE] Upgrade Pub/Sub samples. GoogleCloudPlatform/ruby-docs-samples#63

Closed

jmdobry mentioned this pull request Sep 26, 2016

[DO NOT MERGE] Upgrade pubsub samples. GoogleCloudPlatform/python-docs-samples#543

Closed

jmdobry force-pushed the pubsub-sample-style branch from 66babd4 to 21fb5ab Compare September 26, 2016 18:31

jmdobry assigned ace-n Sep 26, 2016

ace-n reviewed Sep 27, 2016

View reviewed changes

Upgrade pubsub samples.

f74c020

Address comments.

eae0a6f

jmdobry force-pushed the pubsub-sample-style branch from 21fb5ab to eae0a6f Compare September 27, 2016 19:02

Remove extraneous datastore test that's causing things to fail.

4d8e931

ace-n mentioned this pull request Sep 27, 2016

Adds 6 quickstart examples #218

Merged

jmdobry merged commit 35b153f into master Sep 27, 2016

jmdobry deleted the pubsub-sample-style branch September 27, 2016 22:22

pattishin mentioned this pull request Nov 9, 2022

migrate code from googleapis/nodejs-game-servers #2818

Closed

pattishin mentioned this pull request Nov 9, 2022

migrate code from googleapis/nodejs-game-servers #2819

Closed

This was referenced Nov 9, 2022

migrate code from googleapis/nodejs-game-servers #2820

Closed

migrate code from googleapis/nodejs-game-servers #2837

Merged

ace-n pushed a commit that referenced this pull request Nov 16, 2022

docs(samples): Adds start of region tag (#216)

527274d

docs(samples): Adds start of region tag

ace-n mentioned this pull request Nov 16, 2022

migrate code from googleapis/nodejs-video-intelligence #2855

Merged

ace-n pushed a commit that referenced this pull request Nov 17, 2022

docs(samples): Adds start of region tag (#216)

deb95e9

docs(samples): Adds start of region tag

ahrarmonsur pushed a commit that referenced this pull request Nov 17, 2022

docs(samples): Adds start of region tag (#216)

68f0cb3

docs(samples): Adds start of region tag

NimJay pushed a commit that referenced this pull request Nov 19, 2022

chore: release 1.1.0 (#216)

368832d

* updated CHANGELOG.md * updated package.json * updated samples/package.json

NimJay mentioned this pull request Nov 19, 2022

migrate code from googleapis/nodejs-dataproc #2878

Merged

NimJay pushed a commit that referenced this pull request Nov 19, 2022

chore: release 1.1.0 (#216)

bf19208

* updated CHANGELOG.md * updated package.json * updated samples/package.json

kweinmeister mentioned this pull request Jan 11, 2023

migrate code from googleapis/nodejs-retail #2975

Merged

kweinmeister pushed a commit that referenced this pull request Jan 11, 2023

samples: removed region tags from events and search directories (#216)

f032a03

Conversation

jmdobry commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 90.28% (diff: 100%)

Uh oh!

ace-n left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdobry commented Sep 27, 2016

Uh oh!

ace-n commented Sep 27, 2016

Uh oh!

jmdobry commented Sep 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmdobry commented Sep 26, 2016 •

edited

Loading

codecov-io commented Sep 26, 2016 •

edited

Loading