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.

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

Revamp Closure JsUnit tests runtime and optimize test/build flows. #1137

Merged
merged 1 commit into from
Sep 21, 2021

Conversation

sampajano
Copy link
Collaborator

@sampajano sampajano commented Sep 21, 2021

Key changes:

  • Removed dependency on Bazel / rules_closure for Closure JsUnit tests in favor of google-closure-deps and Protractor.

    • This allow us the direct use use most up-to-date version of Closure Library (v.s. indirectly used through rules_closure), and unblock code sync and new releases.
      • Also ensures tests are ran on the same version of Closure as release (which was not before).
    • Protractor setup uses generated test HTML files, and is HEAVILY inspired by FirebaseUi (mainly just Dockerizing their solution) :)
  • Optimized Test flows

    • Restructured Dockerfile layouts and test scripts so that unit tests runs earlier and much faster!
      • e.g. when user makes a javascript changes, reruns will fail in as early as 10 seconds (on jsunit failures) and at most in ~60 seconds (on mocha test failures), compared to 4+ minutes as before ! (on 2019 MacbookPro)
    • npm test in packages/grpc-web now runs both JsUnit tests on top of Mocha tests.
  • Optimized Dockerfiles

    • Removed common Docker images (saves disk space) and revamped prereqs to use multistage build.
      • Avoids the possibility code from previous git clone to be accidentally in local builds
    • Optimized prereqs for faster build
      • Reduced build targets so prereqs build time is down from 5+ minutes to <3 minutes (on 2019 MacbookPro)
      • Optimized build sequence so larger / less frequently changed targets are built earlier to best leverage the Docker build cache.

Other (small) changes:

  • Fix small flake in interop test where it sometimes times out at 500ms (updated timeout to 1 second)
    • .. and updated mocha unit tests timeout to 10 seconds
  • Cleaned up echo example Makefile to ignore testing files (rather than removing then adding them back)
  • Created .dockerignore to avoid build failures when local node_modules/ are present
  • Deleted streambodyclientreadablestream.js since it’s not used and doesn’t build.

Follow-up:

  • Clean up remaining closure_js_library usages and our bazel rules.

@sampajano sampajano marked this pull request as ready for review September 21, 2021 20:00
@sampajano sampajano force-pushed the 1-npm-test branch 2 times, most recently from 46d1f3b to c538d68 Compare September 21, 2021 21:12
stanley-cheung
stanley-cheung previously approved these changes Sep 21, 2021
Copy link
Collaborator

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

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

This is great work! Thanks for modernizing the build and test flows. Thanks for doing all the research into how other JS projects do this, distill and simplify the workflow to match our use case. This will unblock the effort to sync our codebase and work towards finally be able to make another release!

net/grpc/gateway/docker/node_interop_server/Dockerfile Outdated Show resolved Hide resolved
net/grpc/gateway/docker/prereqs/Dockerfile Outdated Show resolved Hide resolved
net/grpc/gateway/docker/prereqs/Dockerfile Show resolved Hide resolved
packages/grpc-web/docker/jsunit-test/Dockerfile Outdated Show resolved Hide resolved
@sampajano
Copy link
Collaborator Author

Thanks so much for the review, Stanley!! Happy to improve things! :)

@sampajano sampajano merged commit 32fe124 into grpc:master Sep 21, 2021
@sampajano sampajano deleted the 1-npm-test branch September 21, 2021 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants