test: add E2E tests for bru.variables, bru.environment, bru.globals APIs#7888
Open
sanish-bruno wants to merge 5 commits intousebruno:mainfrom
Open
test: add E2E tests for bru.variables, bru.environment, bru.globals APIs#7888sanish-bruno wants to merge 5 commits intousebruno:mainfrom
sanish-bruno wants to merge 5 commits intousebruno:mainfrom
Conversation
…and bru.variables methods This commit modifies the translation logic to replace deprecated Postman API calls with updated bru methods for environment and variable management. The changes include updating method names in both the translation files and the corresponding test cases to ensure consistency and correctness in the translation process.
…slations and implementation This commit comments out the globals.unset and globals.clear methods in the Postman translations and the Bru class implementation, marking them as TODOs to be re-enabled once the UI sync issue is resolved. This change ensures that the code remains functional while addressing the current limitations in the UI.
This commit updates the order of sync read methods in the bru.js shims for variables, environment, and globals to ensure a consistent structure across the code. The changes enhance readability and maintainability without altering functionality.
sanish-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
WalkthroughThis PR replaces legacy variable helpers with namespaced Bruno PropertyList APIs and wires them through runtime and QuickJS shims: Changes
Sequence Diagram(s)sequenceDiagram
participant POST as Postman Script (pm.*)
participant TRAN as Postman→Bruno Translator
participant VM as QuickJS Shim (globalThis.bru)
participant BRU as Bru VariableList runtime
participant STORE as PersistentEnvStore
POST->>TRAN: pm.environment.get/set/has/etc.
TRAN->>VM: emit bru.environment.get/set/has/... (translated code)
VM->>BRU: invoke bru.environment.set/get/has/unset/clear
BRU->>STORE: (onSet with persist=true) persist key/value
BRU-->>VM: return values/undefined
VM-->>POST: translated script observes returned value/result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sanish-bruno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru (1)
14-16: Use suite-scoped key names to reduce cross-run collisions.These tests use generic global keys. In persisted/shared runs, collisions can make the missing-key assertion environment-dependent.
♻️ Suggested key hardening
- bru.globals.set("testGlobalVar", "global-hello-123"); - bru.globals.set("testGlobalObj", { scope: "global", active: true }); + bru.globals.set("__bru_globals_get_set_var__", "global-hello-123"); + bru.globals.set("__bru_globals_get_set_obj__", { scope: "global", active: true }); - expect(bru.globals.get("testGlobalVar")).to.equal("global-hello-123"); + expect(bru.globals.get("__bru_globals_get_set_var__")).to.equal("global-hello-123"); - const obj = bru.globals.get("testGlobalObj"); + const obj = bru.globals.get("__bru_globals_get_set_obj__"); - expect(bru.globals.get("nonexistent")).to.be.undefined; + expect(bru.globals.get("__bru_globals_get_set_missing__")).to.be.undefined; - bru.globals.set("testGlobalVar", "updated-456"); - expect(bru.globals.get("testGlobalVar")).to.equal("updated-456"); + bru.globals.set("__bru_globals_get_set_var__", "updated-456"); + expect(bru.globals.get("__bru_globals_get_set_var__")).to.equal("updated-456");As per coding guidelines "Ensure tests are deterministic and reproducible. No randomness, timing dependencies, or environment-specific assumptions without explicit control."
Also applies to: 29-31, 34-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru` around lines 14 - 16, Replace the generic global keys with suite-scoped unique names to avoid cross-run collisions: change the bru.globals.set calls that use "testGlobalVar" and "testGlobalObj" to deterministic, suite-prefixed keys (for example use a constant like "get-set::testGlobalVar" and "get-set::testGlobalObj" or include the test/suite name as a prefix) and update any matching bru.globals.get/clear references elsewhere in this file (the other instances around the same test) to use the new names so all set/get/clear operations remain consistent.packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru (1)
14-16: Use test-scoped global keys to reduce collision risk.Consider prefixing keys (e.g.
__bru_test_globals_iteration_gA) so this request is safer in shared/global environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru` around lines 14 - 16, Replace the plain global keys "gA", "gB", "gC" with test-scoped, prefixed keys to avoid collisions in shared environments; update the bru.globals.set calls to use a unique prefix such as "__bru_test_globals_iteration_" (e.g. "__bru_test_globals_iteration_gA") for each key and ensure any reads of these globals elsewhere in this test reference the same prefixed names (look for bru.globals.set and bru.globals.get usages in this iteration test).packages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.js (1)
108-116: Add a fail-fast guard for invalidsyncWriteMethodsentries.A typo in method config currently surfaces as an opaque runtime error. A small guard gives clearer diagnostics.
Proposed hardening
for (const methodName of syncWriteMethods) { + if (typeof nativeList[methodName] !== 'function') { + throw new Error(`createPropertyListBridge: "${methodName}" is not a function on nativeList`); + } const fn = vm.newFunction(methodName, (...vmArgs) => { const args = vmArgs.map((a) => vm.dump(a)); nativeList[methodName](...args); return vm.undefined; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.js` around lines 108 - 116, The loop that creates sync write wrappers using syncWriteMethods can call undefined native methods if the config has a typo; update the block that iterates syncWriteMethods (the for...of creating functions via vm.newFunction and calling nativeList[methodName]) to validate each methodName up front: if typeof nativeList[methodName] !== "function" throw a clear Error mentioning the invalid methodName and context (e.g., property-list-bridge syncWriteMethods for targetObj), otherwise proceed to create the vm.newFunction wrapper that calls nativeList[methodName]; this ensures a fail-fast, descriptive error instead of an opaque runtime crash.packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru (1)
14-15: Use namespaced global keys to reduce cross-test interference.Because globals are shared and teardown is currently unavailable, generic keys like
gX/gYcan collide with other scripts. Prefer unique test-scoped keys (for example,__cr_globals_readMethods_gX).As per coding guidelines "Ensure tests are deterministic and reproducible. No randomness, timing dependencies, or environment-specific assumptions without explicit control."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru` around lines 14 - 15, The globals use generic keys ("gX" and "gY") which can collide across tests; change calls to bru.globals.set that use "gX" and "gY" to use test-scoped, namespaced keys (e.g. prefix with "__cr_globals_readMethods_") so the keys are unique and reduce cross-test interference while keeping the same values and usage in any corresponding bru.globals.get calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-js/src/bru.js`:
- Around line 90-99: The onSet handler in bru.js currently checks if
(this.persistentEnvVariables[key]) before deleting, which fails for falsy string
values like '' and leaves stale persisted entries; update the deletion check in
the onSet callback to test for the key's presence (e.g., use
Object.prototype.hasOwnProperty.call(this.persistentEnvVariables, key) or the
`in` operator) so that any existing persisted key (even if its value is an empty
string) is removed from this.persistentEnvVariables.
In `@packages/bruno-js/src/variable-list.js`:
- Around line 32-33: The get() method currently reads directly from
_variablesObj and can return keys that should be excluded by options.filterKeys;
update get() to respect the same exclusion logic used by has()/all()/toObject()
by consulting the filtered view instead of _variablesObj directly — e.g., check
the filterKeys set (or reuse this.toObject()/this.all()) and return undefined if
the requested key is in filterKeys, otherwise return the value from the filtered
object; ensure any internal references to _variablesObj in get() are replaced
with the filtered lookup so behavior is consistent across get(), has(), all(),
and toObject().
- Around line 71-79: The set method on VariableList directly assigns keys after
calling `#validateKey`, but the current regex allows prototype-related reserved
keys like "__proto__", "constructor", and "prototype", enabling prototype
pollution; update the key validation logic in VariableList.#validateKey (and the
shared validateKey in bru.js) to explicitly reject reserved property names (at
least "__proto__", "prototype", "constructor") and throw an Error when
encountered instead of allowing assignment, so variables/environment/globals
cannot mutate prototypes; keep the existing regex check but add a reserved-key
blacklist check and reuse the same guarded validation in all places that call
validateKey.
---
Nitpick comments:
In `@packages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.js`:
- Around line 108-116: The loop that creates sync write wrappers using
syncWriteMethods can call undefined native methods if the config has a typo;
update the block that iterates syncWriteMethods (the for...of creating functions
via vm.newFunction and calling nativeList[methodName]) to validate each
methodName up front: if typeof nativeList[methodName] !== "function" throw a
clear Error mentioning the invalid methodName and context (e.g.,
property-list-bridge syncWriteMethods for targetObj), otherwise proceed to
create the vm.newFunction wrapper that calls nativeList[methodName]; this
ensures a fail-fast, descriptive error instead of an opaque runtime crash.
In `@packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru`:
- Around line 14-16: Replace the generic global keys with suite-scoped unique
names to avoid cross-run collisions: change the bru.globals.set calls that use
"testGlobalVar" and "testGlobalObj" to deterministic, suite-prefixed keys (for
example use a constant like "get-set::testGlobalVar" and
"get-set::testGlobalObj" or include the test/suite name as a prefix) and update
any matching bru.globals.get/clear references elsewhere in this file (the other
instances around the same test) to use the new names so all set/get/clear
operations remain consistent.
In `@packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru`:
- Around line 14-16: Replace the plain global keys "gA", "gB", "gC" with
test-scoped, prefixed keys to avoid collisions in shared environments; update
the bru.globals.set calls to use a unique prefix such as
"__bru_test_globals_iteration_" (e.g. "__bru_test_globals_iteration_gA") for
each key and ensure any reads of these globals elsewhere in this test reference
the same prefixed names (look for bru.globals.set and bru.globals.get usages in
this iteration test).
In `@packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru`:
- Around line 14-15: The globals use generic keys ("gX" and "gY") which can
collide across tests; change calls to bru.globals.set that use "gX" and "gY" to
use test-scoped, namespaced keys (e.g. prefix with "__cr_globals_readMethods_")
so the keys are unique and reduce cross-test interference while keeping the same
values and usage in any corresponding bru.globals.get calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d307d3e-d66e-4916-8943-579d466874c8
📒 Files selected for processing (44)
packages/bruno-converters/src/postman/postman-translations.jspackages/bruno-converters/src/utils/postman-to-bruno-translator.jspackages/bruno-converters/tests/postman/postman-translations/postman-comments.spec.jspackages/bruno-converters/tests/postman/postman-translations/postman-edge-cases.spec.jspackages/bruno-converters/tests/postman/postman-translations/postman-variables.spec.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/combined.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/environment.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/legacy-tests-syntax.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/multiline-syntax.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/postman-references.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/request.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/response.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/testing-framework.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variable-chaining.test.jspackages/bruno-converters/tests/postman/postman-translations/transpiler-tests/variables.test.jspackages/bruno-js/src/bru.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jspackages/bruno-js/src/sandbox/quickjs/utils/property-list-bridge.jspackages/bruno-js/src/variable-list.jspackages/bruno-js/tests/variable-list.spec.jspackages/bruno-tests/collection/scripting/api/bru/environment/folder.brupackages/bruno-tests/collection/scripting/api/bru/environment/get-set.brupackages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/environment/iteration.brupackages/bruno-tests/collection/scripting/api/bru/environment/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/globals/folder.brupackages/bruno-tests/collection/scripting/api/bru/globals/get-set.brupackages/bruno-tests/collection/scripting/api/bru/globals/has.brupackages/bruno-tests/collection/scripting/api/bru/globals/iteration.brupackages/bruno-tests/collection/scripting/api/bru/globals/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/variables/folder.brupackages/bruno-tests/collection/scripting/api/bru/variables/get-set.brupackages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/variables/iteration.brupackages/bruno-tests/collection/scripting/api/bru/variables/readMethods.brutests/scripting/bru-api/environment/environment.spec.tstests/scripting/bru-api/environment/init-user-data/collection-security.jsontests/scripting/bru-api/environment/init-user-data/preferences.jsontests/scripting/bru-api/globals/globals.spec.tstests/scripting/bru-api/globals/init-user-data/collection-security.jsontests/scripting/bru-api/globals/init-user-data/preferences.jsontests/scripting/bru-api/variables/init-user-data/collection-security.jsontests/scripting/bru-api/variables/init-user-data/preferences.jsontests/scripting/bru-api/variables/variables.spec.ts
Comment on lines
+90
to
+99
| onSet: (key, value, options) => { | ||
| if (options?.persist) { | ||
| if (typeof value !== 'string') { | ||
| throw new Error(`Persistent environment variables must be strings. Received ${typeof value} for key "${key}".`); | ||
| } | ||
| this.persistentEnvVariables[key] = value; | ||
| } else if (this.persistentEnvVariables[key]) { | ||
| delete this.persistentEnvVariables[key]; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fix stale persisted env cleanup for falsy string values.
At Line 96, truthy checking misses existing persisted keys whose value is '', so stale persisted entries are not removed.
🛠️ Proposed fix
- } else if (this.persistentEnvVariables[key]) {
+ } else if (Object.hasOwn(this.persistentEnvVariables, key)) {
delete this.persistentEnvVariables[key];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-js/src/bru.js` around lines 90 - 99, The onSet handler in
bru.js currently checks if (this.persistentEnvVariables[key]) before deleting,
which fails for falsy string values like '' and leaves stale persisted entries;
update the deletion check in the onSet callback to test for the key's presence
(e.g., use Object.prototype.hasOwnProperty.call(this.persistentEnvVariables,
key) or the `in` operator) so that any existing persisted key (even if its value
is an empty string) is removed from this.persistentEnvVariables.
Comment on lines
+32
to
+33
| * @param {string[]} [options.filterKeys] - Internal keys to exclude from reads (e.g. ['__name__']) | ||
| * @param {Function} [options.onSet] - Callback after set: (key, value, options) => void |
Contributor
There was a problem hiding this comment.
get() currently bypasses filterKeys contract.
filterKeys is documented as exclusion from reads, but Line 60 reads _variablesObj directly and can return filtered keys that has()/all()/toObject() intentionally hide.
♻️ Suggested consistency fix
get(key) {
- return this._interpolateFn(this._variablesObj[key]);
+ const value = super.get(key);
+ return this._interpolateFn(value);
}Also applies to: 59-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-js/src/variable-list.js` around lines 32 - 33, The get()
method currently reads directly from _variablesObj and can return keys that
should be excluded by options.filterKeys; update get() to respect the same
exclusion logic used by has()/all()/toObject() by consulting the filtered view
instead of _variablesObj directly — e.g., check the filterKeys set (or reuse
this.toObject()/this.all()) and return undefined if the requested key is in
filterKeys, otherwise return the value from the filtered object; ensure any
internal references to _variablesObj in get() are replaced with the filtered
lookup so behavior is consistent across get(), has(), all(), and toObject().
Comment on lines
+71
to
+79
| set(key, value, options) { | ||
| if (!key) { | ||
| throw new Error('Creating a variable without specifying a name is not allowed.'); | ||
| } | ||
| this.#validateKey(key); | ||
| this._variablesObj[key] = value; | ||
| if (this._onSet) { | ||
| this._onSet(key, value, options); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspect VariableList validation and write path:"
rg -n -C3 --type=js 'variableNameRegex|#validateKey|set\(|_variablesObj\[key\]' packages/bruno-js/src/variable-list.js
echo
echo "Inspect validateKey provider(s) passed into VariableList:"
rg -n -C4 --type=js 'new VariableList|validateKey' packages/bruno-js/src/bru.js packages/bruno-js/srcRepository: usebruno/bruno
Length of output: 1776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List the repository structure to understand access
echo "Checking repository access and file existence:"
if [ -f "packages/bruno-js/src/variable-list.js" ]; then
echo "✓ File found at packages/bruno-js/src/variable-list.js"
echo
echo "=== Inspecting variable-list.js (lines 1-120) ==="
head -120 packages/bruno-js/src/variable-list.js | cat -n
else
echo "✗ File not found, attempting to locate it:"
find . -name "variable-list.js" -type f 2>/dev/null || echo "Not found in search"
fiRepository: usebruno/bruno
Length of output: 4860
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for VariableList instantiation and validateKey usage ==="
rg -n "new VariableList" packages/bruno-js/src --type=js -B2 -A5
echo
echo "=== Checking for any other prototype pollution protections ==="
rg -n "__proto__|constructor|prototype" packages/bruno-js/src --type=js | grep -i "validate\|forbid\|block\|check" || echo "No related protections found"
echo
echo "=== Verify lines 103-114 context (secondary location mentioned in review) ==="
sed -n '100,120p' packages/bruno-js/src/variable-list.js | cat -nRepository: usebruno/bruno
Length of output: 3232
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect the validateKey function definition in bru.js ==="
rg -n "validateKey\s*=" packages/bruno-js/src/bru.js -B10 -A10
echo
echo "=== Check if there are other VariableList usages outside bru.js ==="
rg -n "new VariableList\|VariableList(" packages/bruno-js/src --type=js | grep -v "bru.js\|variable-list.js"
echo
echo "=== Full context of validateKey in bru.js (lines 60-95) ==="
sed -n '60,95p' packages/bruno-js/src/bru.js | cat -nRepository: usebruno/bruno
Length of output: 1139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if there are other VariableList usages outside bru.js ==="
rg -n "new VariableList" packages/bruno-js/src --type=js
echo
echo "=== Verify the validateKey function in bru.js does NOT block reserved keys ==="
sed -n '72,79p' packages/bruno-js/src/bru.js | cat -n
echo
echo "=== Check what variableNameRegex is imported as in bru.js ==="
head -20 packages/bruno-js/src/bru.js | cat -nRepository: usebruno/bruno
Length of output: 2254
Block prototype-related keys in variable validation.
The regex /^[\w-.]*$/ allows reserved keys like __proto__, constructor, and prototype. All three VariableList instances (variables, environment, globals) inherit this vulnerability since the validateKey function in bru.js uses only regex validation without reserved key checks. Direct assignment at line 76 creates real prototype mutation risk.
Suggested fix
const variableNameRegex = /^[\w-.]*$/;
+const forbiddenVariableKeys = new Set(['__proto__', 'constructor', 'prototype']);
@@
`#validateKey`(key) {
+ if (forbiddenVariableKeys.has(key)) {
+ throw new Error(`Variable name: "${key}" is not allowed.`);
+ }
if (this._validateKeyFn) {
this._validateKeyFn(key);
return;
}Also update bru.js validateKey function similarly, or delegate reserved key check to #validateKey.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-js/src/variable-list.js` around lines 71 - 79, The set method
on VariableList directly assigns keys after calling `#validateKey`, but the
current regex allows prototype-related reserved keys like "__proto__",
"constructor", and "prototype", enabling prototype pollution; update the key
validation logic in VariableList.#validateKey (and the shared validateKey in
bru.js) to explicitly reject reserved property names (at least "__proto__",
"prototype", "constructor") and throw an Error when encountered instead of
allowing assignment, so variables/environment/globals cannot mutate prototypes;
keep the existing regex check but add a reserved-key blacklist check and reuse
the same guarded validation in all places that call validateKey.
This commit introduces a new method `getGlobalEnvName` in the Bru class to retrieve the global environment name. Additionally, it updates the Bru shims to expose this method and define a property for the global environment name, enhancing the accessibility of environment information within the application.
This commit introduces additional error handling mechanisms in the bru.js shims, specifically targeting variable and environment management. The changes aim to prevent potential runtime errors and improve the overall stability of the application without affecting existing functionality.
sanish-bruno
force-pushed
the
tests/variables-scripting-apis
branch
from
b4cf838 to
de50718
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-tests/collection/scripting/api/bru/environment/iteration.bru`:
- Around line 45-48: The test currently calls Array.reduce on the local items
variable (created by bru.environment.filter), so it never exercises the new
PropertyList API; change the test to call bru.environment.reduce directly (use
the same reducer function and initial value), e.g. replace the items.reduce(...)
call with bru.environment.reduce(function(acc, i) { return acc +
Number(i.value); }, 0) inside the "reduce() should accumulate values" test so
bru.environment.reduce is executed and covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f97a998c-d6e6-443e-ad8b-7c306e360f18
📒 Files selected for processing (27)
packages/bruno-app/src/utils/collections/index.jspackages/bruno-js/src/bru.jspackages/bruno-js/src/sandbox/quickjs/shims/bru.jspackages/bruno-tests/collection/scripting/api/bru/environment/folder.brupackages/bruno-tests/collection/scripting/api/bru/environment/get-set.brupackages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/environment/iteration.brupackages/bruno-tests/collection/scripting/api/bru/environment/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/globals/folder.brupackages/bruno-tests/collection/scripting/api/bru/globals/get-set.brupackages/bruno-tests/collection/scripting/api/bru/globals/has.brupackages/bruno-tests/collection/scripting/api/bru/globals/iteration.brupackages/bruno-tests/collection/scripting/api/bru/globals/readMethods.brupackages/bruno-tests/collection/scripting/api/bru/variables/folder.brupackages/bruno-tests/collection/scripting/api/bru/variables/get-set.brupackages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.brupackages/bruno-tests/collection/scripting/api/bru/variables/iteration.brupackages/bruno-tests/collection/scripting/api/bru/variables/readMethods.brutests/scripting/bru-api/environment/environment.spec.tstests/scripting/bru-api/environment/init-user-data/collection-security.jsontests/scripting/bru-api/environment/init-user-data/preferences.jsontests/scripting/bru-api/globals/globals.spec.tstests/scripting/bru-api/globals/init-user-data/collection-security.jsontests/scripting/bru-api/globals/init-user-data/preferences.jsontests/scripting/bru-api/variables/init-user-data/collection-security.jsontests/scripting/bru-api/variables/init-user-data/preferences.jsontests/scripting/bru-api/variables/variables.spec.ts
✅ Files skipped from review due to trivial changes (17)
- tests/scripting/bru-api/environment/init-user-data/collection-security.json
- packages/bruno-tests/collection/scripting/api/bru/variables/folder.bru
- tests/scripting/bru-api/variables/init-user-data/collection-security.json
- packages/bruno-tests/collection/scripting/api/bru/environment/folder.bru
- tests/scripting/bru-api/variables/variables.spec.ts
- packages/bruno-tests/collection/scripting/api/bru/globals/has.bru
- tests/scripting/bru-api/globals/init-user-data/preferences.json
- packages/bruno-tests/collection/scripting/api/bru/variables/has-unset-clear.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/get-set.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/readMethods.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/readMethods.bru
- packages/bruno-tests/collection/scripting/api/bru/environment/get-set.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/folder.bru
- packages/bruno-tests/collection/scripting/api/bru/globals/get-set.bru
- tests/scripting/bru-api/globals/globals.spec.ts
- tests/scripting/bru-api/variables/init-user-data/preferences.json
- packages/bruno-tests/collection/scripting/api/bru/environment/readMethods.bru
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/scripting/bru-api/globals/init-user-data/collection-security.json
- tests/scripting/bru-api/environment/init-user-data/preferences.json
- tests/scripting/bru-api/environment/environment.spec.ts
- packages/bruno-tests/collection/scripting/api/bru/globals/iteration.bru
- packages/bruno-tests/collection/scripting/api/bru/variables/iteration.bru
- packages/bruno-js/src/bru.js
- packages/bruno-tests/collection/scripting/api/bru/environment/has-unset-clear.bru
Comment on lines
+45
to
+48
| test("reduce() should accumulate values", function() { | ||
| const items = bru.environment.filter(function(i) { return i.key.startsWith("iter"); }); | ||
| const sum = items.reduce(function(acc, i) { return acc + Number(i.value); }, 0); | ||
| expect(sum).to.equal(6); |
Contributor
There was a problem hiding this comment.
reduce() API is not being tested directly.
Line [47] uses Array reduce on items, so this test bypasses bru.environment.reduce() and misses coverage for the new PropertyList method.
Proposed fix
- test("reduce() should accumulate values", function() {
- const items = bru.environment.filter(function(i) { return i.key.startsWith("iter"); });
- const sum = items.reduce(function(acc, i) { return acc + Number(i.value); }, 0);
- expect(sum).to.equal(6);
- });
+ test("reduce() should accumulate values", function() {
+ const sum = bru.environment.reduce(function(acc, i) {
+ return i.key.startsWith("iter") ? acc + Number(i.value) : acc;
+ }, 0);
+ expect(sum).to.equal(6);
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("reduce() should accumulate values", function() { | |
| const items = bru.environment.filter(function(i) { return i.key.startsWith("iter"); }); | |
| const sum = items.reduce(function(acc, i) { return acc + Number(i.value); }, 0); | |
| expect(sum).to.equal(6); | |
| test("reduce() should accumulate values", function() { | |
| const sum = bru.environment.reduce(function(acc, i) { | |
| return i.key.startsWith("iter") ? acc + Number(i.value) : acc; | |
| }, 0); | |
| expect(sum).to.equal(6); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-tests/collection/scripting/api/bru/environment/iteration.bru`
around lines 45 - 48, The test currently calls Array.reduce on the local items
variable (created by bru.environment.filter), so it never exercises the new
PropertyList API; change the test to call bru.environment.reduce directly (use
the same reducer function and initial value), e.g. replace the items.reduce(...)
call with bru.environment.reduce(function(acc, i) { return acc +
Number(i.value); }, 0) inside the "reduce() should accumulate values" test so
bru.environment.reduce is executed and covered.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA
Summary
Follow-up to #7887. Adds
.bruE2E test files and Playwright specs for the newbru.variables,bru.environment, andbru.globalsPropertyList APIs.Test coverage
Three new test folders under
packages/bruno-tests/collection/scripting/api/bru/:variables/get-set,has-unset-clear,readMethods,iterationenvironment/get-set(incl.namegetter),has-unset-clear(preserves__name__),readMethods(excludes__name__fromtoObject),iterationglobals/get-set,has(incl.namegetter, verifiesunset/clearthrow "not implemented"),readMethods,iterationEach test file covers:
bru.environment.nameandbru.globals.nameE2E Playwright specs
Three new spec files under
tests/scripting/bru-api/(mirroring the existingcookies/pattern):variables/variables.spec.ts— runs in both developer and safe (QuickJS) modeenvironment/environment.spec.ts— runs in both developer and safe modeglobals/globals.spec.ts— runs in both developer and safe modeEach spec runs the corresponding
.brufolder and validates all 4 requests pass in both sandbox modes.Contribution Checklist:
Summary by CodeRabbit
New Features
bru.environment.*,bru.variables.*, andbru.globals.*withget(),set(),has(),unset(), andclear()methods for improved variable handling and clarity.Tests