Skip to content

Add TestModuleDifferentSizes#395

Closed
webmaster128 wants to merge 2 commits intobuke:mainfrom
webmaster128:test-load-module
Closed

Add TestModuleDifferentSizes#395
webmaster128 wants to merge 2 commits intobuke:mainfrom
webmaster128:test-load-module

Conversation

@webmaster128
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented May 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.96%. Comparing base (196eb54) to head (b12412e).
Report is 59 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #395      +/-   ##
==========================================
- Coverage   86.11%   80.96%   -5.15%     
==========================================
  Files           6        5       -1     
  Lines         641      825     +184     
==========================================
+ Hits          552      668     +116     
- Misses         71      123      +52     
- Partials       18       34      +16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@buke
Copy link
Owner

buke commented Jul 23, 2025

/gemini review

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

The code changes introduce a new test, TestModuleDifferentSizes, to validate the loading of JavaScript modules of varying sizes. My review focuses on improving the test's efficiency and maintainability. I suggest using a more efficient method for string building, enabling parallel test execution, and enhancing code clarity by removing commented-out code and magic numbers.

}

func TestModuleDifferentSizes(t *testing.T) {
memoryLimit := uint64(1 * 1024 * 1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The memory limit is defined using a magic number (1 * 1024 * 1024). It's better to define this as a named constant to improve readability and maintainability.

Suggested change
memoryLimit := uint64(1 * 1024 * 1024)
const memoryLimit = 1 * 1024 * 1024

Comment on lines +920 to +924
// Uncomment for failures
// "5k": 5000,
// "6k": 6000,
// "7k": 7000,
// "8k": 8000,
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The test contains commented-out test cases. This can lead to confusion and code clutter. If these cases are intended to fail, they should be structured as proper test cases that assert an error. If they are for temporary debugging, they should be removed before merging the code.

Comment on lines +927 to +943
for name, locs := range specs {
t.Run(name, func(t *testing.T) {
rt := quickjs.NewRuntime()
defer rt.Close()
rt.SetMemoryLimit(memoryLimit)
ctx := rt.NewContext()
defer ctx.Close()

code := ""
for i := 0; i < locs; i++ {
code += fmt.Sprintf(`export var myvar%d = "foo%d";`, i, i) + "\n"
}

result, err := ctx.LoadModule(code, name+".js")
require.NoError(t, err)
defer result.Free()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The sub-tests within this table-driven test can be run in parallel to potentially speed up the test suite execution. To do this safely, you should call t.Parallel() and also capture the range variables (name and locs) within the sub-test's scope to prevent race conditions.

t.Run(name, func(t *testing.T) {
			t.Parallel()
			// capture range variables to run tests in parallel safely
			name := name
			locs := locs
			rt := quickjs.NewRuntime()
			defer rt.Close()
			rt.SetMemoryLimit(memoryLimit)
			ctx := rt.NewContext()
			defer ctx.Close()

			code := ""
			for i := 0; i < locs; i++ {
				code += fmt.Sprintf(`export var myvar%d = \"foo%d\";`, i, i) + "\n"
			}

			result, err := ctx.LoadModule(code, name+".js")
			require.NoError(t, err)
			defer result.Free()
		})

Comment on lines +936 to +938
for i := 0; i < locs; i++ {
code += fmt.Sprintf(`export var myvar%d = "foo%d";`, i, i) + "\n"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

Using += to build a string inside a loop is inefficient as it causes multiple memory allocations and copies. For better performance, especially with a large number of lines (locs), it's recommended to use strings.Builder.

var code strings.Builder
			for i := 0; i < locs; i++ {
				code.WriteString(fmt.Sprintf(`export var myvar%d = \"foo%d\";\n`, i, i))
			}

			result, err := ctx.LoadModule(code.String(), name+".js")

@buke buke closed this Aug 1, 2025
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.

2 participants