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

x/pkgsite: "// Output:" matcher for test Examples is noncanonical #65450

Closed
adonovan opened this issue Feb 2, 2024 · 3 comments
Closed

x/pkgsite: "// Output:" matcher for test Examples is noncanonical #65450

adonovan opened this issue Feb 2, 2024 · 3 comments
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. pkgsite

Comments

@adonovan
Copy link
Member

adonovan commented Feb 2, 2024

The standard logic for extracting "// Output:" comments in example tests is here:
https://cs.opensource.google/go/go/+/master:src/go/doc/example.go;l=113;drc=28f1bf61b7383bd4079d77090e67b3198b75be12
Observe that it has a ^ anchor so that it matches only the start of the comment text.
By contrast, pksite uses this regexp:
https://cs.opensource.google/go/x/pkgsite/+/master:internal/godoc/dochtml/internal/render/render.go;l=24;drc=7431aa5932c48bce425bf3f2dc368c5b1fed6e2c
which lacks the anchor. The consequence is that an example such as this one:

func Example() {
    // A
    // B This Example does not have an "// Output:" comment so it will not run.
    // C
}

is considered executable by pkgsite, and its rendering of the example is abruptly cut off at line C.

@gopherbot gopherbot added this to the Unreleased milestone Feb 2, 2024
@adonovan adonovan changed the title x/pkgsite: "// Output:" matcher in test Examples is noncanonical x/pkgsite: "// Output:" matcher for test Examples is noncanonical Feb 2, 2024
@findleyr findleyr modified the milestones: Unreleased, pkgsite/backlog Feb 8, 2024
@wdhongtw
Copy link

This issue can be detected by adding one test case

diff --git a/internal/godoc/dochtml/internal/render/linkify_test.go b/internal/godoc/dochtml/internal/render/linkify_test.go
index d3ea8b83..7b455204 100644
--- a/internal/godoc/dochtml/internal/render/linkify_test.go
+++ b/internal/godoc/dochtml/internal/render/linkify_test.go
@@ -426,6 +426,18 @@ a := 1
 // Output:
 b := 1
 </pre>
+`,
+               },
+               {
+                       "Special \"Output\" comment must on it's own comment block",
+                       `_ = true
+// To test output, use: // output: the output content
+`,
+                       `
+<pre class="Documentation-exampleCode">
+_ = true
+// To test output, use: // output: the output content
+</pre>
 `,
                },
        } {

And can be fixed quickly by ensuring there is only spaces before the // sequence

diff --git a/internal/godoc/dochtml/internal/render/render.go b/internal/godoc/dochtml/internal/render/render.go
index 3875f3ba..0ad66c9e 100644
--- a/internal/godoc/dochtml/internal/render/render.go
+++ b/internal/godoc/dochtml/internal/render/render.go
@@ -21,7 +21,7 @@ import (
 
 var (
        // Regexp for example outputs.
-       exampleOutputRx = regexp.MustCompile(`(?i)//[[:space:]]*(unordered )?output:`)
+       exampleOutputRx = regexp.MustCompile(`(?i)^[[:space:]]*//[[:space:]]*(unordered )?output:`)
 )
 
 type Renderer struct {

Since that there could only be space-like characters before the // Output: block, assuming the the code is formatted.

@findleyr Would you think this issue deserve a fix? If so, maybe I can submit a PR for this. :D

@adonovan
Copy link
Member Author

@wdhongtw a fix would be much appreciated; thanks for offering.

wdhongtw added a commit to wdhongtw/pkgsite that referenced this issue Jul 23, 2024
Current logic for detecting "concluding line comment" is not strict
enough that it may cause false-positive, causing some normal comment
(and code following it) to be discarded in the rendered web page.

Ensure that "concluding line comment" can only be prefixed by spaces.
Also add corresponding test case.

Fixes golang/go#65450

Change-Id: Ief9ec5326aa94965ca02d3278398cc5663ba395f
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/600415 mentions this issue: internal/godoc/dochtml: enhance output parsing for example func

wdhongtw added a commit to wdhongtw/pkgsite that referenced this issue Jul 23, 2024
Current logic for detecting "concluding line comment" is not strict
enough that it may cause false-positive, causing some normal comment
(and code following it) to be discarded in the rendered web page.

Ensure that "concluding line comment" can only be prefixed by spaces.
Also add corresponding test case.

Fixes golang/go#65450

Change-Id: I764c6aebb20e8cacf4ea127e9195b80c7bb24f0e
wdhongtw added a commit to wdhongtw/pkgsite that referenced this issue Sep 13, 2024
Current logic for detecting "concluding line comment" is not strict
enough that it may cause false-positive, causing some normal comment
(and code following it) to be discarded in the rendered web page.

Ensure that "concluding line comment" can only be prefixed by spaces.
Also add corresponding test case.

Fixes golang/go#65450

Change-Id: Iab6cfdb4e7ada8fe8fe1b2b2aa38dea8f4c7067c
wdhongtw added a commit to wdhongtw/pkgsite that referenced this issue Sep 13, 2024
Current logic for detecting "concluding line comment" is not strict
enough that it may cause false-positive, causing some normal comment
(and code following it) to be discarded in the rendered web page.

Ensure that "concluding line comment" can only be prefixed by spaces.
Also add corresponding test case.

Fixes golang/go#65450

Change-Id: Ibb27edb7a6addbb255e4eeffe9c533862b1c0db1
@dmitshur dmitshur added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted NeedsFix The path to resolution is known, but the work has not been done. pkgsite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants