Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

feat: typescript conversion on video-intelligence #375

Merged
merged 24 commits into from
Mar 9, 2020
Merged

feat: typescript conversion on video-intelligence #375

merged 24 commits into from
Mar 9, 2020

Conversation

summer-ji-eng
Copy link
Contributor

@summer-ji-eng summer-ji-eng commented Mar 3, 2020 •
edited
Loading

docs-test fails due to broken link, fixing in https://critique-ng.corp.google.com/cl/298705338

Other than that, it is ready to review.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 3, 2020
@codecov
Copy link

codecov bot commented Mar 3, 2020 •
edited
Loading

Codecov Report

Merging #375 into master will increase coverage by 64.27%.
The diff coverage is 83.10%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #375       +/-   ##
===========================================
+ Coverage   25.65%   89.93%   +64.27%     
===========================================
  Files          38       12       -26     
  Lines        9135     2463     -6672     
  Branches        0      154      +154     
===========================================
- Hits         2344     2215      -129     
+ Misses       6791      243     -6548     
- Partials        0        5        +5
Impacted Files Coverage Δ
src/index.ts 100.00% <0.00%> (ø)
src/v1p3beta1/index.ts 100.00% <0.00%> (ø)
src/v1/index.ts 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b46112...2317d54. Read the comment docs.

xiaozhenliu-gg5
xiaozhenliu-gg5 reviewed Mar 5, 2020
View reviewed changes
package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
protos/google/cloud/videointelligence/v1beta2/video_intelligence.proto Show resolved Hide resolved
samples/analyze_person_detection.js Outdated Show resolved Hide resolved
src/index.ts
const VideoIntelligenceServiceClient = v1.VideoIntelligenceServiceClient;
const StreamingVideoIntelligenceServiceClient =
v1p3beta1.StreamingVideoIntelligenceServiceClient;
export {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for confirmation, we want StreamingVideoIntelligenceServiceClient from v1p3beta1 as default, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StreamingVideoIntelligenceServiceClient is new Client introduced in v1p3beta1. I am not 100% sure if we want put it in export default. @alexander-fenster could you give me suggestion? Thanks

@summer-ji-eng summer-ji-eng added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 5, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 5, 2020
@xiaozhenliu-gg5
Copy link
Contributor

Seems these two tests fail for samples-test:

at Context.it (system-test/analyze_person_detection_gcs.test.js:29:12)
analyzing people in video
    1) should identify people in a local file
    2) should identify people in a file in Google Storage

And there's assertion error, that means, it got some results which is different from the expectation. Please debug a little bit, it might be useful to trace back when it starts to fail, and what changes did you make. Thank you!

@summer-ji-eng
Copy link
Contributor Author 

  1) VideoIntelligenceServiceSmokeTest
       successfully makes a call to the service:
     Error: Timeout of 10000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/tmpfs/src/github/nodejs-video-intelligence/build/system-test/video_intelligence_service_smoke_test.js)

Sometime the system test passed and sometimes failed. It fails because call to const client = new videoIntelligence.v1p1beta1.VideoIntelligenceServiceClient exceed 10000ms.
@alexander-fenster any suggestion here? Thanks

@alexander-fenster alexander-fenster mentioned this pull request Mar 6, 2020
Closed
JustinBeckwith
JustinBeckwith reviewed Mar 6, 2020
View reviewed changes
synth.py
)

# skip index, protos, package.json, and README.md
s.copy(library, excludes=["package.json", "README.md", "src/index.js", "smoke-test/video_intelligence_service_smoke_test.js"])
s.copy(library, excludes=["package.json", "README.md", "src/index.ts",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we still generating smoke tests like this? Seems snowflakey :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a real "smoke test", it's a system test that is not named properly. @summer-ji-eng The new generator does not make any smoke tests, so we should just "upgrade" the generated smoke test to a system test and commit it into system-tests folder once and forever.

JustinBeckwith
JustinBeckwith approved these changes Mar 6, 2020
View reviewed changes
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

LGTM w/ nit

@summer-ji-eng summer-ji-eng merged commit 30b01d5 into master Mar 9, 2020
@summer-ji-eng summer-ji-eng deleted the summer_typescript_conversion branch March 9, 2020 04:35
@summer-ji-eng summer-ji-eng mentioned this pull request Mar 9, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@xiaozhenliu-gg5 xiaozhenliu-gg5 xiaozhenliu-gg5 left review comments

@JustinBeckwith JustinBeckwith JustinBeckwith approved these changes

@alexander-fenster alexander-fenster Awaiting requested review from alexander-fenster

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@summer-ji-eng @xiaozhenliu-gg5 @JustinBeckwith @alexander-fenster @googlebot @yoshi-kokoro