internal/testing: add an integration test for pkg/directory resolution

A new integration test, TestModulePackageDirectoryResolution is added to
test the resolution of modules vs. packages vs. directories when a path
is only a valid package a certain versions.

In order for this test to pass I had to fix the return code when we
suggest a search: previously we were returning StatusSeeOther, but that
should really only be used when we're actually redirecting to a
different page via the location header. Instead, we now return
StatusNotFound.

A convenience function htmlcheck.Run is added for running htmlcheck on
the Body of a net/http.Response.

Updates b/143814014
Fixes b/145858138
Updates b/144031201
Updates b/143760329

Change-Id: Ie3f02165a6607b341379a1aea2eb078f300ad405
Reviewed-on: https://team-review.git.corp.google.com/c/golang/discovery/+/614811
CI-Result: Cloud Build <devtools-proctor-result-processor@system.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>

internal/frontend/details.go

@@ -96,6 +96,13 @@ func (s *Server) servePackagePage(w http.ResponseWriter, r *http.Request, pkgPat
 			s.serveDirectoryPage(w, r, pkgPath, modulePath, version)
 			return
 		}
+		// Temporary patch: StatusNotFound should not be used unless we're setting
+		// the Location header, but we can't set it earlier or we'll change the
+		// handling logic.
+		// TODO(b/144031201): remove this after refactoring.
+		if code == http.StatusSeeOther {
+			code = http.StatusNotFound
+		}
 		s.serveErrorPage(w, r, code, epage)
 		return
 	}
@@ -160,6 +167,13 @@ func (s *Server) serveModulePage(w http.ResponseWriter, r *http.Request, moduleP
 		return modulePath, err
 	})
 	if code != http.StatusOK {
+		// Temporary patch: StatusNotFound should not be used unless we're setting
+		// the Location header, but we can't set it earlier or we'll change the
+		// handling logic.
+		// TODO(b/144031201): remove this after refactoring.
+		if code == http.StatusSeeOther {
+			code = http.StatusNotFound
+		}
 		s.serveErrorPage(w, r, code, epage)
 		return
 	}

internal/testing/htmlcheck/htmlcheck.go

@@ -8,6 +8,7 @@ package htmlcheck
 
 import (
 	"fmt"
+	"io"
 	"regexp"
 	"strings"
 
@@ -18,6 +19,16 @@ import (
 // A Checker is a function from an HTML node to an error describing a failure.
 type Checker func(*html.Node) error
 
+// Run is a convenience function to run the checker against HTML read from
+// reader.
+func Run(reader io.Reader, checker Checker) error {
+	node, err := html.Parse(reader)
+	if err != nil {
+		return err
+	}
+	return checker(node)
+}
+
 // In returns a Checker that applies the given checkers to the first node
 // matching the CSS selector. The empty selector denotes the entire subtree of
 // the Checker's argument node.

internal/testing/integration/frontend_test.go

@@ -0,0 +1,186 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package integration
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"golang.org/x/discovery/internal/etl"
+	"golang.org/x/discovery/internal/frontend"
+	"golang.org/x/discovery/internal/middleware"
+	"golang.org/x/discovery/internal/postgres"
+	"golang.org/x/discovery/internal/proxy"
+	"golang.org/x/discovery/internal/testing/htmlcheck"
+	"golang.org/x/discovery/internal/testing/testhelper"
+)
+
+var (
+	in      = htmlcheck.In
+	hasText = htmlcheck.HasText
+)
+
+func TestModulePackageDirectoryResolution(t *testing.T) {
+	// The shared test state sets up the following scenario to exercise the types
+	// of problems discussed in b/143814014: a directory that becomes a package,
+	// and then becomes a directory again. Specifically:
+	//  + at v1.2.3, github.com/golang/found/dir is a directory (containing dir/pkg)
+	//  + at v1.2.4, github.com/golang/found/dir is a package
+	//  + at v1.2.5, github.com/golang/found/dir is again just a directory
+	versions := []*proxy.TestVersion{
+		proxy.NewTestVersion(t, "github.com/golang/found", "v1.2.3", map[string]string{
+			"found.go":       "package found\nconst Value = 123",
+			"dir/pkg/pkg.go": "package pkg\nconst Value = 321",
+			"LICENSE":        testhelper.MITLicense,
+		}),
+		proxy.NewTestVersion(t, "github.com/golang/found", "v1.2.4", map[string]string{
+			"found.go":       "package found\nconst Value = 124",
+			"dir/pkg/pkg.go": "package pkg\nconst Value = 421",
+			"dir/dir.go":     "package dir\nconst Value = \"I'm a package!\"",
+			"LICENSE":        testhelper.MITLicense,
+		}),
+		proxy.NewTestVersion(t, "github.com/golang/found", "v1.2.5", map[string]string{
+			"found.go":       "package found\nconst Value = 125",
+			"dir/pkg/pkg.go": "package pkg\nconst Value = 521",
+			"LICENSE":        testhelper.MITLicense,
+		}),
+	}
+
+	tests := []struct {
+		// Test description.
+		desc string
+		// URL Path (relative to the test server) to check
+		urlPath string
+		// The expected HTTP status code.
+		wantCode int
+		// If non-nil, used to verify the resulting page.
+		want htmlcheck.Checker
+	}{
+		{
+			desc:     "missing module",
+			urlPath:  "/mod/github.com/golang/missing",
+			wantCode: http.StatusNotFound,
+		},
+		{
+			desc:     "latest module",
+			urlPath:  "/mod/github.com/golang/found",
+			wantCode: http.StatusOK,
+			want:     in(".DetailsHeader", hasText("v1.2.5")),
+		},
+		{
+			desc:     "versioned module",
+			urlPath:  "/mod/github.com/golang/found@v1.2.3",
+			wantCode: http.StatusOK,
+			want:     in(".DetailsHeader", hasText("v1.2.3")),
+		},
+		{
+			desc:     "non-existent version",
+			urlPath:  "/mod/github.com/golang/found@v1.1.3",
+			wantCode: http.StatusNotFound,
+			want: in(".Content",
+				hasText("not available"),
+				hasText("other versions of this module")),
+		},
+		{
+			desc:     "latest package",
+			urlPath:  "/github.com/golang/found/dir/pkg",
+			wantCode: http.StatusOK,
+			want: in("",
+				in(".DetailsHeader", hasText("v1.2.5")),
+				in(".DetailsContent", hasText("521"))),
+		},
+		{
+			desc:     "earlier package",
+			urlPath:  "/github.com/golang/found@v1.2.3/dir/pkg",
+			wantCode: http.StatusOK,
+			want: in("",
+				in(".DetailsHeader", hasText("v1.2.3")),
+				in(".DetailsContent", hasText("321"))),
+		},
+		// This test fails, because the fetchPackageOrModule logic instead returns
+		// 404 and suggests a search for the later package.
+		// TODO(b/143814014): uncomment once this bug is fixed.
+		// {
+		// 	desc:     "dir is initially a directory",
+		// 	urlPath:  "/github.com/golang/found@v1.2.3/dir",
+		// 	wantCode: http.StatusOK,
+		// 	want:     in(".Directories", hasText("pkg")),
+		// },
+		{
+			desc:     "dir becomes a package",
+			urlPath:  "/github.com/golang/found@v1.2.4/dir",
+			wantCode: http.StatusOK,
+			want: in("",
+				in(".DetailsHeader", hasText("v1.2.4")),
+				in(".DetailsContent", hasText("I'm a package"))),
+		},
+		// This test fails, because fetchPackageOrMOdule again suggests a search
+		// for the (now earlier) package.
+		// TODO(b/143814014): uncomment once this bug is fixed.
+		// {
+		// 	desc:     "dir becomes a directory again",
+		// 	urlPath:  "/github.com/golang/found@v1.2.5/dir",
+		// 	wantCode: http.StatusOK,
+		// 	want:     in(".Directories", hasText("pkg")),
+		// },
+		// This test fails, because we currently go to the latest version of
+		// found/dir, which is v1.2.4. We should instead serve the directory from
+		// the latest version of the module
+		// TODO(b/143814014): uncomment once this bug is fixed.
+		// {
+		// 	desc:     "latest directory",
+		// 	urlPath:  "/github.com/golang/found/dir",
+		// 	wantCode: http.StatusOK,
+		// 	want:     in(".Directories", hasText("pkg")),
+		// },
+	}
+	s, err := frontend.NewServer(testDB, nil, "../../../content/static", false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	mux := http.NewServeMux()
+	s.Install(mux.Handle, nil)
+	handler := middleware.LatestVersion(s.LatestVersion)(mux)
+	ts := httptest.NewServer(handler)
+	processVersions(context.Background(), t, versions)
+	defer postgres.ResetTestDB(testDB, t)
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			resp, err := http.Get(ts.URL + test.urlPath)
+			if err != nil {
+				t.Fatal(err)
+			}
+			defer resp.Body.Close()
+			if resp.StatusCode != test.wantCode {
+				t.Errorf("GET %s returned status %d, want %d", test.urlPath, resp.StatusCode, test.wantCode)
+			}
+			if test.want != nil {
+				if err := htmlcheck.Run(resp.Body, test.want); err != nil {
+					t.Error(err)
+				}
+			}
+		})
+	}
+
+}
+
+func processVersions(ctx context.Context, t *testing.T, testVersions []*proxy.TestVersion) {
+	t.Helper()
+	proxyClient, teardown := proxy.SetupTestProxy(t, testVersions)
+	defer teardown()
+
+	for _, tv := range testVersions {
+		v, _, err := etl.FetchVersion(ctx, tv.ModulePath, tv.Version, proxyClient)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if err := testDB.InsertVersion(ctx, v); err != nil {
+			t.Fatal(err)
+		}
+	}
+}