test(optimiser): add unit tests and fix linting for optimiser

.circleci/config.yml

@@ -143,20 +143,26 @@ jobs:
           command: cp ../../src/data/venues.json _constants/venues.json
       - run:
           name: Install golangci-lint
-          command: curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.7.2
+          command: curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.11.4
       - run:
-          name: Check linting
-          command: $(go env GOPATH)/bin/golangci-lint run --timeout=5m
+          name: Lint code
+          command: $(go env GOPATH)/bin/golangci-lint run --timeout=5m . ./_*/
       - run:
-          name: Check formatting
+          name: Lint styles
           command: |
             if [ -n "$($(go env GOPATH)/bin/golangci-lint fmt --diff)" ]; then
-              echo "Formatting issues found. Run 'golangci-lint fmt' locally to fix."
+              echo "Formatting issues found. Run 'pnpm lint:optimiser:styles' locally to fix."
               exit 1
             fi
       - run:
           name: Run tests
           command: |
+            for dir in ./_*/; do
+              if [ "$dir" != "./_test/" ]; then
+                go test -count=1 -v "$dir"
+              fi
+            done
+
             go run ./_test/server/main.go -port 8020 &
             SERVER_PID=$!
 
@@ -169,6 +175,8 @@ jobs:
 
             go test ./_test/. -v
 
+
+
 workflows:
   build_and_test:
     jobs:

website/api/optimiser/.golangci.yaml

@@ -66,10 +66,10 @@ linters:
     - funcorder # checks the order of functions, methods, and constructors
     - funlen # tool for detection of long functions
     - gocheckcompilerdirectives # validates go compiler directive comments (//go:)
-    - gochecknoglobals # checks that no global variables exist
+    # - gochecknoglobals # checks that no global variables exist
     - gochecknoinits # checks that no init functions are present in Go code
     - gochecksumtype # checks exhaustiveness on Go "sum types"
-    - gocognit # computes and checks the cognitive complexity of functions
+    # - gocognit # computes and checks the cognitive complexity of functions
     - goconst # finds repeated strings that could be replaced by a constant
     - gocritic # provides diagnostics that check for bugs, performance and style issues
     - gocyclo # computes and checks the cyclomatic complexity of functions
@@ -86,7 +86,7 @@ linters:
     - loggercheck # checks key value pairs for common logger libraries (kitlog,klog,logr,zap)
     - makezero # finds slice declarations with non-zero initial length
     - mirror # reports wrong mirror patterns of bytes/strings usage
-    - mnd # detects magic numbers
+    # - mnd # detects magic numbers
     - modernize # suggests simplifications to Go code, using modern language and library features
     - musttag # enforces field tags in (un)marshaled structs
     - nakedret # finds naked returns in functions greater than a specified function length
@@ -128,7 +128,7 @@ linters:
     #- decorder # checks declaration order and count of types, constants, variables and functions
     #- exhaustruct # [highly recommend to enable] checks if all structure fields are initialized
     #- ginkgolinter # [if you use ginkgo/gomega] enforces standards of using ginkgo and gomega
-    #- godox # detects usage of FIXME, TODO and other keywords inside comments
+    # - godox # detects usage of FIXME, TODO and other keywords inside comments
     #- goheader # checks is file header matches to pattern
     #- inamedparam # [great idea, but too strict, need to ignore a lot of cases by default] reports interfaces with unnamed method parameters
     #- interfacebloat # checks the number of methods inside an interface
@@ -288,7 +288,7 @@ linters:
       # Checks the number of statements in a function.
       # If lower than 0, disable the check.
       # Default: 40
-      statements: 50
+      statements: 60
 
     gochecksumtype:
       # Presence of `default` case in switch statements satisfies exhaustiveness, if all members are not listed.
@@ -469,3 +469,6 @@ linters:
           - gosec
           - noctx
           - wrapcheck
+          - govet
+          - testpackage
+          - unparam

website/api/optimiser/README.md

@@ -265,7 +265,7 @@ The scoring function combines four penalty/bonus terms. All values were empirica
    pnpm start:optimiser
    ```
 
-4. **Run frontend**(if needed)
+4. **Run frontend** (if needed)
 
    ```bash
    pnpm start:local
@@ -275,25 +275,38 @@ The scoring function combines four penalty/bonus terms. All values were empirica
 
 - Send a POST request following the request body format above to `http://localhost:8020/optimise`
 - Or run the integration tests (requires the test server to be running):
+
   ```bash
   go test ./_test/... -v
   ```
 
 ## Linting and Formatting
 
+### Linting
+
 - Lint the code using:
+
   ```bash
-  golangci-lint run
+  pnpm lint:optimiser:code
   ```
+
   _Auto fix issues where possible:_
+
   ```bash
-  golangci-lint run --fix
+  pnpm lint:optimiser:code --fix
   ```
+
+- The linter will help to do static analysis and catch common issues. The configuration is defined in `.golangci.yaml`.
+
+### Formatting
+
 - Format the code using:
+
   ```bash
-  golangci-lint fmt
+  pnpm lint:optimiser:styles
   ```
-- The golangci-lint configuration is defined in `.golangci.yaml`
+
+- This will format the code according to the defined style guide. The configuration is defined in `.golangci.yaml`.
 
 ## Dependencies
 
@@ -328,4 +341,4 @@ The slot merging step in `_modules/mergeAndFilterModuleSlots` uses the building
 - Tweak the scoring function to prioritise more important constraints found from user feedback. For instance:
 - Tweak the beam search parameters to improve performance (perhaps depending on the number of modules)
 - Create a more accurate heuristic for scoring distance between consecutive classes. (Currently, it just a random linear function that seems to work)
-- Add more constraints to optimisation proceess
+- Add more constraints to optimisation process

website/api/optimiser/_client/client.go

@@ -1,4 +1,4 @@
-// HTTP Client to make requests to NUSMODS apis
+// Package client makes HTTP requests to the NUSMods API.
 package client
 
 import (
@@ -12,10 +12,10 @@ import (
 
 var httpClient = &http.Client{Timeout: 10 * time.Second}
 
-// HTTP request to get Module data
+// GetModuleData fetches timetable data for a module from the NUSMods API.
 func GetModuleData(acadYear string, module string) ([]byte, error) {
 	url := fmt.Sprintf(constants.ModulesURL, acadYear, module)
-	res, err := httpClient.Get(url)
+	res, err := httpClient.Get(url) //nolint:noctx // httpClient has a timeout set
 	if err != nil {
 		return nil, err
 	}

website/api/optimiser/_constants/constants.go

@@ -6,7 +6,7 @@ import (
 	models "github.com/nusmodifications/nusmods/website/api/optimiser/_models"
 )
 
-// Ensure in sync with all E-Venues in NUSMods
+// EVenues lists all virtual venue identifiers. Keep in sync with all E-Venues in NUSMods.
 var EVenues = map[string]struct{}{
 	"E-Learn_A":  {},
 	"E-Learn_B":  {},
@@ -18,7 +18,7 @@ var EVenues = map[string]struct{}{
 	"E-Hybrid_D": {},
 }
 
-// Ensure this is in sync with website/src/utils/timetables.ts
+// LessonTypeAbbrev maps full lesson type names to their abbreviations. Keep in sync with website/src/utils/timetables.ts.
 var LessonTypeAbbrev = map[string]string{
 	"DESIGN LECTURE":             "DLEC",
 	"LABORATORY":                 "LAB",
@@ -35,13 +35,13 @@ var LessonTypeAbbrev = map[string]string{
 }
 
 //go:embed venues.json
-var VenuesJson []byte
+var VenuesJSON []byte
 
 const ModulesURL = "https://api.nusmods.com/v2/%s/modules/%s.json"
 
 const NUSModsTimetableBaseURL = "https://nusmods.com/timetable"
 
-// Heuristics for scoring function
+// Heuristics for scoring function.
 const (
 	MaxWalkDistance             = 0.250 // 250 meters
 	NoVenuePenalty              = 100.0
@@ -53,15 +53,15 @@ const (
 	ConsecutiveHoursPenaltyRate = 100
 )
 
-// This is used by [nusmods_link.SerializeLessonIndices] to serialize the result of optimiser into a timetable share link to return to the client
+// ModuleCodeSeparator is used by [nusmods_link.SerializeLessonIndices] to serialize the optimiser result into a timetable share link.
 const ModuleCodeSeparator = ";"
 
-// Beam search parameters
+// Beam search parameters.
 const (
 	BeamWidth       = 5000
 	BranchingFactor = 100
 	DaysPerWeek     = 6
 )
 
-// Indicates that a Coordinate was invalid
+// InvalidCoordinates indicates that a venue coordinate is invalid.
 var InvalidCoordinates = models.Coordinates{X: -1, Y: -1}

website/api/optimiser/_models/models.go

@@ -1,6 +1,7 @@
 package models
 
 import (
+	"errors"
 	"fmt"
 	"strconv"
 	"strings"
@@ -10,10 +11,10 @@ type LessonType = string
 type ClassNo = string
 type LessonIndex = int
 
-// ModuleTimetableMap organises module slots by Module -> LessonType -> ClassNo -> []ModuleSlot
+// ModuleTimetableMap organises module slots by Module -> LessonType -> ClassNo -> []ModuleSlot.
 type ModuleTimetableMap = map[string]map[LessonType]map[ClassNo][]ModuleSlot
 
-// ModuleDefaultSlotsMap organises default/backup slots by Module -> LessonType -> []ModuleSlot
+// ModuleDefaultSlotsMap organises default/backup slots by Module -> LessonType -> []ModuleSlot.
 type ModuleDefaultSlotsMap = map[string]map[LessonType][]ModuleSlot
 
 type OptimiserRequest struct {
@@ -38,7 +39,7 @@ type OptimiserRequest struct {
 // ParseOptimiserRequestFields validates and parses time fields into minutes.
 func (r *OptimiserRequest) ParseOptimiserRequestFields() error {
 	if len(r.Modules) == 0 {
-		return fmt.Errorf("at least one module must be provided")
+		return errors.New("at least one module must be provided")
 	}
 	var err error
 	r.EarliestMin, err = ParseTimeToMinutes(r.EarliestTime)
@@ -65,7 +66,7 @@ func (r *OptimiserRequest) ParseOptimiserRequestFields() error {
 }
 
 // SolveError is returned by Solve to communicate both the error message and the
-// appropriate HTTP status code to the handler
+// appropriate HTTP status code to the handler.
 type SolveError struct {
 	Code    int
 	Message string
@@ -75,6 +76,7 @@ func (e *SolveError) Error() string { return e.Message }
 
 type SolveResponse struct {
 	TimetableState
+
 	ShareableLink        string `json:"shareableLink"`
 	DefaultShareableLink string `json:"defaultShareableLink"`
 }
@@ -109,7 +111,7 @@ type ModuleSlot struct {
 	LessonIndex LessonIndex      `json:"LessonIndex"`
 }
 
-// ParseModuleSlotFields parses and populates the parsed fields in ModuleSlot for faster computation
+// ParseModuleSlotFields parses and populates the parsed fields in ModuleSlot for faster computation.
 func (slot *ModuleSlot) ParseModuleSlotFields(lessonKey string) error {
 	startMin, err1 := ParseTimeToMinutes(slot.StartTime)
 	endMin, err2 := ParseTimeToMinutes(slot.EndTime)
@@ -156,13 +158,13 @@ func ParseTimeToMinutes(timeStr string) (int, error) {
 	if len(timeStr) != 4 {
 		return 0, fmt.Errorf("invalid time format: %s", timeStr)
 	}
-	hour, err1 := strconv.Atoi(timeStr[:2])
-	min, err2 := strconv.Atoi(timeStr[2:])
+	hours, err1 := strconv.Atoi(timeStr[:2])
+	mins, err2 := strconv.Atoi(timeStr[2:])
 	if err1 != nil || err2 != nil {
 		return 0, fmt.Errorf("invalid time format: %s", timeStr)
 	}
-	if hour < 0 || hour > 23 || min < 0 || min > 59 {
+	if hours < 0 || hours > 23 || mins < 0 || mins > 59 {
 		return 0, fmt.Errorf("time out of range: %s", timeStr)
 	}
-	return hour*60 + min, nil
+	return hours*60 + mins, nil
 }