Skip to content

Conversation

santigimeno
Copy link
Member

@santigimeno santigimeno commented Sep 18, 2025

Summary by CodeRabbit

  • Chores
    • Updated test environment prerequisites to install the latest gRPC proto-loader dependency rather than a pinned version.
    • Reduces maintenance overhead from strict version pinning and aligns test setup with current tooling.
    • No changes to application behavior or user-facing functionality; this is a development/test maintenance update.

@santigimeno santigimeno self-assigned this Sep 18, 2025
Copy link

coderabbitai bot commented Sep 18, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Updated the Makefile’s test-agents-prereqs target to install the latest @grpc/proto-loader via npm instead of pinning it to version 0.7. The @grpc/grpc-js install remains unchanged. No other targets or commands were modified.

Changes

Cohort / File(s) Summary
Build/test prerequisites
Makefile
In target test-agents-prereqs, changed npm i @grpc/grpc-js @grpc/[email protected] to npm i @grpc/grpc-js @grpc/proto-loader. No other edits.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • RafaelGSS
  • juanarbol

Poem

I nudge the Makefile, light and neat,
Unpin a version—a tiny feat.
Proto-loader hops to latest cheer,
My whiskers twitch: “All clear here!”
With tools aligned and caches clean,
I bound ahead through build-step green. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "test: fix flaky test-grpc-packages" is concise and directly reflects the primary intent of the change—stabilizing a flaky gRPC-related test. The Makefile change in the diff updates the npm install of @grpc/proto-loader from a pinned 0.7 to the latest, which is clearly related to gRPC packages and test flakiness. Therefore the title accurately summarizes the main change and will be understandable to reviewers scanning the history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c5c219c and 609557e.

📒 Files selected for processing (1)
  • Makefile (1 hunks)

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
Makefile (2)

1693-1700: Avoid floating “latest” to keep CI deterministic; parameterize or pin versions.

Installing unpinned @grpc/grpc-js and @grpc/proto-loader makes test-agents-prereqs non-reproducible and can reintroduce flakes when those packages release. Recommend parameterizing the specs so you can set/override versions in CI, defaulting to latest only when unset.

Apply this diff within the changed line to use variables:

-env npm_config_nodedir=$(PWD) $(NODE) ./deps/npm install @grpc/grpc-js @grpc/proto-loader --prefix test/common/nsolid-grpc-agent --no-save --no-package-lock
+env npm_config_nodedir=$(PWD) $(NODE) ./deps/npm install $(GRPC_JS_SPEC) $(PROTO_LOADER_SPEC) --prefix test/common/nsolid-grpc-agent --no-save --no-package-lock

Add these variable definitions near the top (outside this hunk), so CI can pin when needed:

# Inherit from environment; when empty, installs latest.
GRPC_JS_VERSION ?=
PROTO_LOADER_VERSION ?=
GRPC_JS_SPEC := @grpc/grpc-js$(if $(GRPC_JS_VERSION),@$(GRPC_JS_VERSION),)
PROTO_LOADER_SPEC := @grpc/proto-loader$(if $(PROTO_LOADER_VERSION),@$(PROTO_LOADER_VERSION),)

1701-1705: Add cleanup for nsolid-grpc-agent node_modules.

test-agents-prereqs installs deps under test/common/nsolid-grpc-agent, but test-agents-prereqs-clean doesn’t remove them.

Apply this diff:

 test-agents-prereqs-clean:
 	$(RM) -r test/common/nsolid-zmq-agent/node_modules
 	$(RM) -r test/common/nsolid-otlp-agent/node_modules
+	$(RM) -r test/common/nsolid-grpc-agent/node_modules
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ae1497d and c5c219c.

📒 Files selected for processing (1)
  • Makefile (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: build-tarball
  • GitHub Check: test-linux (ubuntu-24.04)
  • GitHub Check: lint-js-and-md
  • GitHub Check: test-macOS
  • GitHub Check: test-linux (ubuntu-24.04-arm)

@santigimeno santigimeno changed the base branch from node-v22.x-nsolid-v5.x to node-v22.x-nsolid-v6.x September 23, 2025 08:52
Copy link
Contributor

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

Nice

@santigimeno santigimeno linked an issue Oct 13, 2025 that may be closed by this pull request
@santigimeno santigimeno force-pushed the santi/fix_flaky_test_grpc_packages branch from c5c219c to 609557e Compare October 13, 2025 08:35
santigimeno added a commit that referenced this pull request Oct 13, 2025
Fixes: #378
PR-URL: #368
Reviewed-By: Juan José Arboleda <[email protected]>
@santigimeno
Copy link
Member Author

Landed in 45abe09

@santigimeno santigimeno deleted the santi/fix_flaky_test_grpc_packages branch October 13, 2025 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test-grpc-packages has been failing for a while

2 participants