Addressing in-source TODOs/FIXMEs

programmfabrik · Jun 25, 2024 · f5920d7 · f5920d7
1 parent 24e3c2d
commit f5920d7
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 18 deletions.
diff --git a/api_testsuite.go b/api_testsuite.go
@@ -53,7 +53,7 @@ type Suite struct {
 	reporterRoot    *report.ReportElement
 	index           int
 	serverURL       *url.URL
-	httpServer      http.Server
+	httpServer      *http.Server
 	httpServerProxy *httpproxy.Proxy
 	httpServerDir   string
 	idleConnsClosed chan struct{}
@@ -185,12 +185,11 @@ func (ats *Suite) Run() bool {
 	defer ats.StopSmtpServer()
 
 	ats.StartHttpServer()
-	// FIXME: Defer stop
+	defer ats.StopHttpServer()
 
 	err := os.Chdir(ats.manifestDir)
 	if err != nil {
-		// FIXME: This should be a fatal error - also check other occurrences of Error/Errorf
-		logrus.Errorf("Unable to switch working directory to %q", ats.manifestDir)
+		logrus.Fatalf("Unable to switch working directory to %q", ats.manifestDir)
 	}
 
 	start := time.Now()
@@ -231,8 +230,6 @@ func (ats *Suite) Run() bool {
 		}
 	}
 
-	ats.StopHttpServer()
-
 	return success
 }
 

diff --git a/http_server.go b/http_server.go
@@ -11,6 +11,7 @@ import (
 	"path/filepath"
 	"unicode/utf8"
 
+	"github.com/pkg/errors"
 	"github.com/programmfabrik/apitest/internal/httpproxy"
 	"github.com/programmfabrik/golib"
 	"github.com/sirupsen/logrus"
@@ -19,12 +20,11 @@ import (
 // StartHttpServer start a simple http server that can server local test resources during the testsuite is running
 func (ats *Suite) StartHttpServer() {
 
-	// FIXME: This doesn't check if the http server is already initialized
-	if ats.HttpServer == nil {
+	if ats.HttpServer == nil || ats.httpServer != nil {
 		return
 	}
 
-	// TODO: Find out what idleConnsClosed does and if SMTP server needs it too
+	// TODO: Can we remove idleConnsClosed, because it does not seem to do anything?
 	ats.idleConnsClosed = make(chan struct{})
 	mux := http.NewServeMux()
 
@@ -55,7 +55,7 @@ func (ats *Suite) StartHttpServer() {
 		ats.smtpServer.RegisterRoutes(mux, "/", ats.Config.LogShort)
 	}
 
-	ats.httpServer = http.Server{
+	ats.httpServer = &http.Server{
 		Addr:    ats.HttpServer.Addr,
 		Handler: mux,
 	}
@@ -66,11 +66,9 @@ func (ats *Suite) StartHttpServer() {
 		}
 
 		err := ats.httpServer.ListenAndServe()
-		if err != http.ErrServerClosed { // FIXME: Use errors.Is
+		if !errors.Is(err, http.ErrServerClosed) {
 			// Error starting or closing listener:
-			// FIXME: Use logrus.Fatal
-			logrus.Errorf("HTTP server ListenAndServe: %v", err)
-			return // FIXME: This is redundant
+			logrus.Fatal("HTTP server ListenAndServe:", err)
 		}
 	}
 
@@ -102,12 +100,10 @@ func customStaticHandler(h http.Handler) http.HandlerFunc {
 // StopHttpServer stop the http server that was started for this test suite
 func (ats *Suite) StopHttpServer() {
 
-	if ats.HttpServer == nil {
+	if ats.HttpServer == nil || ats.httpServer == nil {
 		return
 	}
 
-	// FIXME: There is no nil check for ats.httpServer; no protection against calling twice
-
 	err := ats.httpServer.Shutdown(context.Background())
 	if err != nil {
 		// Error from closing listeners, or context timeout:
@@ -117,7 +113,8 @@ func (ats *Suite) StopHttpServer() {
 	} else if !ats.Config.LogShort {
 		logrus.Infof("Http Server stopped: %s", ats.httpServerDir)
 	}
-	return // FIXME: This is redundant
+
+	ats.httpServer = nil
 }
 
 type ErrorResponse struct {

diff --git a/smtp_server.go b/smtp_server.go
@@ -47,4 +47,6 @@ func (ats *Suite) StopSmtpServer() {
 	} else if !ats.Config.LogShort {
 		logrus.Info("SMTP Server stopped")
 	}
+
+	ats.smtpServer = nil
 }