Skip to content

test: cover LLGo directive comments#1834

Merged
cpunion merged 1 commit intoxgo-dev:mainfrom
cpunion:test/directive-comments
Apr 28, 2026
Merged

test: cover LLGo directive comments#1834
cpunion merged 1 commit intoxgo-dev:mainfrom
cpunion:test/directive-comments

Conversation

@cpunion
Copy link
Copy Markdown
Collaborator

@cpunion cpunion commented Apr 27, 2026

Summary

  • add LLGo runtime tests for //go:linkname, //llgo:link, // llgo:link, and //llgo:skip
  • add a build-tagged test/go fixture containing //llgo:skipall so the directive spelling is covered without changing default test package codegen
  • keep this PR test-only; it does not include go:nointerface support or compiler refactoring

Tests

  • go test ./test/go
  • LLGO_BUILD_CACHE=off go run ./cmd/llgo test ./test/go -run 'TestLLGoDirective'

Notes

  • The //llgo:skip test is a link-time regression check: if the skipped function is compiled, it references a deliberately missing C symbol and the package fails to link.
  • The //llgo:skipall fixture is behind an explicit build tag because enabling skipall in the normal test package skips package codegen, including the package init symbol expected by the generated test main.

Copy link
Copy Markdown
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

This pull request introduces test cases for llgo compiler directives, including link, skip, and skipall. Feedback indicates that the skip directive test should be adjusted to prevent dead code elimination from masking failures and that the skipall fixture requires specific build tags to be properly exercised during testing.

Comment thread test/go/directives_test.go Outdated
Comment thread test/go/directives_skipall_fixture_test.go Outdated
Copy link
Copy Markdown
Contributor

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

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

Review Summary

This is a small, focused test-only PR that adds runtime tests for LLGo directive comments (//go:linkname, //llgo:link, //llgo:skip, //llgo:skipall). The //go:linkname and //llgo:link coverage is solid and provides direct, asserted verification. A few quality/consistency points worth addressing:

  • Missing license headers: Other test files in test/go/ (e.g., binop_test.go, cgo_malloc_test.go) carry the standard Apache 2.0 header; both new files omit it.
  • Legacy // +build lines: No other file in the directory uses the old-style constraint; let go fmt manage these or drop them for consistency.
  • Import grouping: The standalone import _ "unsafe" should be folded into the grouped block — this matches the convention in promoted_offsetof_test.go, string_slice_base_test.go, etc.
  • Passive skip tests: TestLLGoDirectiveSkip is empty and the skipall fixture has no driver — both rely entirely on link-time failure as evidence. This is a valid technique but warrants explanatory comments and ideally a more active assertion. The fixture file in particular has no obvious harness invoking it with the directive_skipall_fixture tag.
  • Anchor types: directiveSkipAnchor and directiveSkipAllAnchor are unused; a one-line comment explaining they exist as doc-comment anchors for the directive would prevent future "dead code" cleanup.

No security or performance concerns.

Comment thread test/go/directives_test.go Outdated
Comment thread test/go/directives_test.go Outdated
Comment thread test/go/directives_test.go
Comment thread test/go/directives_test.go
Comment thread test/go/directives_test.go
Comment thread test/go/directives_test.go
Comment thread test/go/directives_skipall_fixture_test.go
Comment thread test/go/directives_skipall_fixture_test.go
Comment thread test/go/directives_skipall_fixture_test.go
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.48%. Comparing base (b4d9167) to head (5442f6a).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1834   +/-   ##
=======================================
  Coverage   88.48%   88.48%           
=======================================
  Files          50       50           
  Lines       14418    14418           
=======================================
  Hits        12758    12758           
  Misses       1440     1440           
  Partials      220      220           

☔ 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.

@cpunion cpunion force-pushed the test/directive-comments branch from 02304a8 to 5442f6a Compare April 27, 2026 10:51
@cpunion cpunion merged commit 7ea3148 into xgo-dev:main Apr 28, 2026
90 of 91 checks passed
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.

1 participant