diff --git a/.circleci/config.yml b/.circleci/config.yml index 12df149b51..df289c4bdd 100644 --- a/.circleci/config.yml +++ b/.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: diff --git a/website/api/optimiser/.golangci.yaml b/website/api/optimiser/.golangci.yaml index 7521343328..7eccffa842 100644 --- a/website/api/optimiser/.golangci.yaml +++ b/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 diff --git a/website/api/optimiser/README.md b/website/api/optimiser/README.md index 6b89742dce..21087e609a 100644 --- a/website/api/optimiser/README.md +++ b/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 diff --git a/website/api/optimiser/_client/client.go b/website/api/optimiser/_client/client.go index aeca5f4030..f380c8f556 100644 --- a/website/api/optimiser/_client/client.go +++ b/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 } diff --git a/website/api/optimiser/_constants/constants.go b/website/api/optimiser/_constants/constants.go index 02c3df11a2..87b22f7dab 100644 --- a/website/api/optimiser/_constants/constants.go +++ b/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} diff --git a/website/api/optimiser/_models/models.go b/website/api/optimiser/_models/models.go index f1955d8145..c9e0c53006 100644 --- a/website/api/optimiser/_models/models.go +++ b/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 } diff --git a/website/api/optimiser/_models/models_test.go b/website/api/optimiser/_models/models_test.go new file mode 100644 index 0000000000..711e3a196f --- /dev/null +++ b/website/api/optimiser/_models/models_test.go @@ -0,0 +1,241 @@ +package models + +import ( + "testing" +) + +func TestParseTimeToMinutes(t *testing.T) { + tests := []struct { + name string + input string + want int + wantErr bool + }{ + {"midnight", "0000", 0, false}, + {"9am", "0900", 540, false}, + {"noon", "1200", 720, false}, + {"end of day", "2359", 1439, false}, + {"earliest valid hour", "0000", 0, false}, + {"latest valid hour", "2300", 1380, false}, + {"hour too large", "2400", 0, true}, + {"hour way too large", "9900", 0, true}, + {"minute too large", "0060", 0, true}, + {"minute boundary valid", "0059", 59, false}, + {"too short", "090", 0, true}, + {"too long", "09000", 0, true}, + {"non-numeric hours", "abcd", 0, true}, + {"non-numeric minutes", "09ab", 0, true}, + {"empty string", "", 0, true}, + {"single space", " ", 0, true}, + {"mixed valid", "1530", 15*60 + 30, false}, + {"hour 23 minute 59", "2359", 23*60 + 59, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParseTimeToMinutes(tt.input) + if (err != nil) != tt.wantErr { + t.Errorf("ParseTimeToMinutes(%q) error = %v, wantErr %v", tt.input, err, tt.wantErr) + return + } + if !tt.wantErr && got != tt.want { + t.Errorf("ParseTimeToMinutes(%q) = %d, want %d", tt.input, got, tt.want) + } + }) + } +} + +func TestParseModuleSlotFields_ValidSlots(t *testing.T) { + t.Run("monday slot", func(t *testing.T) { + slot := ModuleSlot{Day: "Monday", StartTime: "0900", EndTime: "1000"} + err := slot.ParseModuleSlotFields("CS1010S|Lecture") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if slot.StartMin != 540 { + t.Errorf("StartMin = %d, want 540", slot.StartMin) + } + if slot.EndMin != 600 { + t.Errorf("EndMin = %d, want 600", slot.EndMin) + } + if slot.DayIndex != 0 { + t.Errorf("DayIndex = %d, want 0", slot.DayIndex) + } + if slot.LessonKey != "CS1010S|Lecture" { + t.Errorf("LessonKey = %q, want CS1010S|Lecture", slot.LessonKey) + } + }) + + t.Run("midnight boundary", func(t *testing.T) { + slot := ModuleSlot{Day: "Friday", StartTime: "0000", EndTime: "0100"} + if err := slot.ParseModuleSlotFields("X|Y"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if slot.StartMin != 0 || slot.EndMin != 60 { + t.Errorf("StartMin=%d EndMin=%d, want 0 60", slot.StartMin, slot.EndMin) + } + }) + + dayTests := []struct { + day string + wantIdx int + }{ + {"Monday", 0}, + {"Tuesday", 1}, + {"Wednesday", 2}, + {"Thursday", 3}, + {"Friday", 4}, + {"Saturday", 5}, + // uppercase variants + {"MONDAY", 0}, + {"SATURDAY", 5}, + // mixed case + {"monday", 0}, + {"friday", 4}, + } + for _, tt := range dayTests { + t.Run("day_"+tt.day, func(t *testing.T) { + slot := ModuleSlot{Day: tt.day, StartTime: "0900", EndTime: "1000"} + if err := slot.ParseModuleSlotFields("X|Y"); err != nil { + t.Fatalf("day %q: unexpected error: %v", tt.day, err) + } + if slot.DayIndex != tt.wantIdx { + t.Errorf("day %q: DayIndex = %d, want %d", tt.day, slot.DayIndex, tt.wantIdx) + } + }) + } +} + +func TestParseModuleSlotFields_InvalidInputs(t *testing.T) { + tests := []struct { + name string + day string + startTime string + endTime string + }{ + {"invalid StartTime letters", "Monday", "abcd", "1000"}, + {"invalid StartTime hour", "Monday", "2500", "1000"}, + {"invalid StartTime length", "Monday", "900", "1000"}, + {"invalid EndTime letters", "Monday", "0900", "wxyz"}, + {"invalid EndTime minute", "Monday", "0900", "0061"}, + {"invalid EndTime length", "Monday", "0900", "10000"}, + {"unknown day Sunday", "Sunday", "0900", "1000"}, + {"empty day", "", "0900", "1000"}, + {"numeric day", "1", "0900", "1000"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + slot := ModuleSlot{Day: tt.day, StartTime: tt.startTime, EndTime: tt.endTime} + if err := slot.ParseModuleSlotFields("X|Y"); err == nil { + t.Errorf("expected error for day=%q start=%q end=%q", tt.day, tt.startTime, tt.endTime) + } + }) + } +} + +func TestParseOptimiserRequestFields(t *testing.T) { + makeValidReq := func() OptimiserRequest { + return OptimiserRequest{ + Modules: []string{"CS1010S"}, + EarliestTime: "0800", + LatestTime: "2000", + LunchStart: "1200", + LunchEnd: "1400", + } + } + + t.Run("valid request parses correctly", func(t *testing.T) { + req := makeValidReq() + if err := req.ParseOptimiserRequestFields(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if req.EarliestMin != 480 { + t.Errorf("EarliestMin = %d, want 480 (08:00)", req.EarliestMin) + } + if req.LatestMin != 1200 { + t.Errorf("LatestMin = %d, want 1200 (20:00)", req.LatestMin) + } + if req.LunchStartMin != 720 { + t.Errorf("LunchStartMin = %d, want 720 (12:00)", req.LunchStartMin) + } + if req.LunchEndMin != 840 { + t.Errorf("LunchEndMin = %d, want 840 (14:00)", req.LunchEndMin) + } + }) + + t.Run("multiple modules accepted", func(t *testing.T) { + req := makeValidReq() + req.Modules = []string{"CS1010S", "CS2040S", "MA1521"} + if err := req.ParseOptimiserRequestFields(); err != nil { + t.Fatalf("unexpected error with multiple modules: %v", err) + } + }) + + t.Run("empty modules returns error", func(t *testing.T) { + req := makeValidReq() + req.Modules = nil + if err := req.ParseOptimiserRequestFields(); err == nil { + t.Error("expected error for nil modules") + } + }) + + t.Run("empty modules slice returns error", func(t *testing.T) { + req := makeValidReq() + req.Modules = []string{} + if err := req.ParseOptimiserRequestFields(); err == nil { + t.Error("expected error for empty modules slice") + } + }) + + t.Run("invalid EarliestTime returns error", func(t *testing.T) { + req := makeValidReq() + req.EarliestTime = "ABCD" + if err := req.ParseOptimiserRequestFields(); err == nil { + t.Error("expected error for non-numeric EarliestTime") + } + }) + + t.Run("out-of-range EarliestTime returns error", func(t *testing.T) { + req := makeValidReq() + req.EarliestTime = "2500" + if err := req.ParseOptimiserRequestFields(); err == nil { + t.Error("expected error for hour=25 EarliestTime") + } + }) + + t.Run("invalid LatestTime returns error", func(t *testing.T) { + req := makeValidReq() + req.LatestTime = "99" + if err := req.ParseOptimiserRequestFields(); err == nil { + t.Error("expected error for short LatestTime") + } + }) + + t.Run("invalid LunchStart returns error", func(t *testing.T) { + req := makeValidReq() + req.LunchStart = "" + if err := req.ParseOptimiserRequestFields(); err == nil { + t.Error("expected error for empty LunchStart") + } + }) + + t.Run("invalid LunchEnd returns error", func(t *testing.T) { + req := makeValidReq() + req.LunchEnd = "abc" + if err := req.ParseOptimiserRequestFields(); err == nil { + t.Error("expected error for non-numeric LunchEnd") + } + }) + + t.Run("midnight EarliestTime is valid", func(t *testing.T) { + req := makeValidReq() + req.EarliestTime = "0000" + if err := req.ParseOptimiserRequestFields(); err != nil { + t.Errorf("unexpected error for midnight EarliestTime: %v", err) + } + if req.EarliestMin != 0 { + t.Errorf("EarliestMin = %d, want 0", req.EarliestMin) + } + }) +} diff --git a/website/api/optimiser/_modules/modules.go b/website/api/optimiser/_modules/modules.go index 0784938401..e18b616d4d 100644 --- a/website/api/optimiser/_modules/modules.go +++ b/website/api/optimiser/_modules/modules.go @@ -37,8 +37,8 @@ func GetAllModuleSlots( // These are default or backup slots for the partial timetable so that we can display some random slot for unallocated lessons defaultSlots := make(models.ModuleDefaultSlotsMap) for _, module := range optimiserRequest.Modules { - - body, err := client.GetModuleData(optimiserRequest.AcadYear, strings.ToUpper(module)) + var body []byte + body, err = client.GetModuleData(optimiserRequest.AcadYear, strings.ToUpper(module)) if err != nil { return nil, nil, nil, err } @@ -68,20 +68,19 @@ func GetAllModuleSlots( // Parse the weeks for i := range moduleTimetable { - // Note: if weeks is not a []int, then skip parsing // Currently we are not handling week conflict for non-[]int weeks - if _, ok := moduleTimetable[i].Weeks.([]any); !ok { + weeks, ok := moduleTimetable[i].Weeks.([]any) + if !ok { continue } moduleTimetable[i].WeeksSet = make(map[int]struct{}) - weeks := moduleTimetable[i].Weeks.([]any) weeksStrings := make([]string, 0, len(weeks)) for _, week := range weeks { - weekFloat, ok := week.(float64) - if !ok { + weekFloat, isFloat := week.(float64) + if !isFloat { continue } weekInt := int(weekFloat) @@ -101,18 +100,17 @@ func GetAllModuleSlots( optimiserRequest.EarliestMin, optimiserRequest.LatestMin, ) - } return moduleSlots, defaultSlots, recordingsMap, nil } -// Gets all venue information from venues.json +// Gets all venue information from venues.json. func getVenues() (map[string]models.Location, error) { venues := make(map[string]models.Location) - err := json.Unmarshal(constants.VenuesJson, &venues) + err := json.Unmarshal(constants.VenuesJSON, &venues) if err != nil { - return nil, fmt.Errorf("Unable to load venues.json: %v", err) + return nil, fmt.Errorf("unable to load venues.json: %w", err) } return venues, nil @@ -127,7 +125,6 @@ func mergeAndFilterModuleSlots( earliestMin int, latestMin int, ) (map[models.LessonType]map[models.ClassNo][]models.ModuleSlot, map[models.LessonType][]models.ModuleSlot) { - // We group by classNo because some slots come as a pair, ie you have to attend both slots to complete the lesson // Key: "lessonType|classNo", Value: []ModuleSlot @@ -159,7 +156,6 @@ func mergeAndFilterModuleSlots( if defaultSlots[lessonType] == nil { defaultSlots[lessonType] = slots } - // Only apply filters to physical lessons if !isRecorded { for i := range slots { @@ -169,7 +165,6 @@ func mergeAndFilterModuleSlots( allValid = false break } - if isSlotOutsideTimeRange(*slot, earliestMin, latestMin) { allValid = false break @@ -218,6 +213,8 @@ func mergeAndFilterModuleSlots( var combinationParts []string allEVenues := true for _, slot := range slots { + // TODO: Remove the skip for E-venues & + // remove the tests for it too if _, ok := constants.EVenues[slot.Venue]; !ok { allEVenues = false buildingName := extractBuildingName(slot.Venue) @@ -225,7 +222,6 @@ func mergeAndFilterModuleSlots( combinationParts = append(combinationParts, part) } } - // If not all venues are E-Venues, check for duplicates if !allEVenues { combinationKey := lessonType + "|" + strings.Join(combinationParts, "|") @@ -240,7 +236,6 @@ func mergeAndFilterModuleSlots( } mergedTimetable[lessonType][classNo] = slots } - return mergedTimetable, defaultSlots } @@ -261,7 +256,7 @@ func extractBuildingName(key string) string { return parts[0] } -// Checks if a venue inside venues.json has missing location +// Checks if a venue inside venues.json has missing location. func isMissingVenueCoordinates(coord models.Coordinates) bool { return coord.X == 0 || coord.Y == 0 } diff --git a/website/api/optimiser/_modules/modules_test.go b/website/api/optimiser/_modules/modules_test.go new file mode 100644 index 0000000000..8952f6f976 --- /dev/null +++ b/website/api/optimiser/_modules/modules_test.go @@ -0,0 +1,393 @@ +package modules + +import ( + "testing" + + constants "github.com/nusmodifications/nusmods/website/api/optimiser/_constants" + models "github.com/nusmodifications/nusmods/website/api/optimiser/_models" +) + +// ────────────────────────────────────────────────── +// extractBuildingName +// ────────────────────────────────────────────────── + +func TestExtractBuildingName(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"BIZ2-0111", "BIZ2"}, + {"COM1-0212", "COM1"}, + {"AS4-0602", "AS4"}, + {"LT17", "LT17"}, // no hyphen — whole string returned + {"", ""}, // empty string + {"A-B-C", "A"}, // only first segment + {"-suffix", ""}, // leading hyphen gives empty first segment + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + got := extractBuildingName(tt.input) + if got != tt.want { + t.Errorf("extractBuildingName(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + +// ────────────────────────────────────────────────── +// isMissingVenueCoordinates +// ────────────────────────────────────────────────── + +func TestIsMissingVenueCoordinates(t *testing.T) { + tests := []struct { + name string + coord models.Coordinates + want bool + }{ + {"both zero", models.Coordinates{X: 0, Y: 0}, true}, + {"X zero Y non-zero", models.Coordinates{X: 0, Y: 1.29}, true}, + {"X non-zero Y zero", models.Coordinates{X: 103.77, Y: 0}, true}, + {"both non-zero", models.Coordinates{X: 103.77, Y: 1.29}, false}, + {"very small non-zero", models.Coordinates{X: 0.0001, Y: 0.0001}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isMissingVenueCoordinates(tt.coord) + if got != tt.want { + t.Errorf("isMissingVenueCoordinates(%v) = %v, want %v", tt.coord, got, tt.want) + } + }) + } +} + +// ────────────────────────────────────────────────── +// isSlotOutsideTimeRange +// ────────────────────────────────────────────────── + +func TestIsSlotOutsideTimeRange(t *testing.T) { + // earliestMin=480 (08:00), latestMin=1020 (17:00) + earliest, latest := 480, 1020 + + tests := []struct { + name string + startTime string + endTime string + want bool + }{ + {"within range", "0900", "1000", false}, + {"starts exactly at earliest", "0800", "0900", false}, + {"ends exactly at latest", "1600", "1700", false}, + {"starts before earliest", "0700", "0800", true}, + {"ends after latest", "1700", "1800", true}, + {"starts before and ends after latest", "0700", "1800", true}, + {"starts at latest (end is fine)", "1700", "1800", true}, // end > latest + {"invalid StartTime returns false", "abcd", "1000", false}, // conservative + {"invalid EndTime returns false", "0900", "wxyz", false}, // conservative + {"empty StartTime returns false", "", "1000", false}, + {"empty EndTime returns false", "0900", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + slot := models.ModuleSlot{StartTime: tt.startTime, EndTime: tt.endTime} + got := isSlotOutsideTimeRange(slot, earliest, latest) + if got != tt.want { + t.Errorf("isSlotOutsideTimeRange(start=%q, end=%q, earliest=%d, latest=%d) = %v, want %v", + tt.startTime, tt.endTime, earliest, latest, got, tt.want) + } + }) + } +} + +// ────────────────────────────────────────────────── +// mergeAndFilterModuleSlots +// ────────────────────────────────────────────────── + +// minVenues creates a minimal venues map for testing. +func minVenues() map[string]models.Location { + return map[string]models.Location{ + "BIZ2-0111": {Location: models.Coordinates{X: 103.7748, Y: 1.2936}}, + "COM1-0212": {Location: models.Coordinates{X: 103.7716, Y: 1.2952}}, + // E-Venue entries + "E-Learn_A": {Location: models.Coordinates{X: 103.77, Y: 1.29}}, + } +} + +// makeRawSlot builds a timetable slot as would come from the NUSMods API. +func makeRawSlot(lessonType, classNo, day, startTime, endTime, venue string) models.ModuleSlot { + return models.ModuleSlot{ + LessonType: lessonType, + ClassNo: classNo, + Day: day, + StartTime: startTime, + EndTime: endTime, + Venue: venue, + } +} + +func TestMergeAndFilterModuleSlots_FreeDayFiltering(t *testing.T) { + venues := minVenues() + freeDays := map[string]struct{}{"Monday": {}} + recordings := map[string]struct{}{} + + timetable := []models.ModuleSlot{ + makeRawSlot("Lecture", "01", "Monday", "0900", "1000", "BIZ2-0111"), + makeRawSlot("Lecture", "02", "Tuesday", "1000", "1100", "BIZ2-0111"), + } + + merged, _ := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 0, 1440) + + lectureGroups := merged["Lecture"] + if _, ok := lectureGroups["01"]; ok { + t.Error("class 01 on Monday (free day) should be filtered out") + } + if _, ok := lectureGroups["02"]; !ok { + t.Error("class 02 on Tuesday should be kept") + } +} + +func TestMergeAndFilterModuleSlots_TimeRangeFiltering(t *testing.T) { + venues := minVenues() + freeDays := map[string]struct{}{} + recordings := map[string]struct{}{} + + // earliestMin=600 (10:00), latestMin=1020 (17:00) + timetable := []models.ModuleSlot{ + makeRawSlot("Lecture", "01", "Tuesday", "0800", "0900", "BIZ2-0111"), // before earliest + makeRawSlot("Lecture", "02", "Tuesday", "1000", "1100", "BIZ2-0111"), // within range + makeRawSlot("Lecture", "03", "Tuesday", "1700", "1800", "BIZ2-0111"), // after latest + } + + merged, _ := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 600, 1020) + + lectureGroups := merged["Lecture"] + if _, ok := lectureGroups["01"]; ok { + t.Error("class 01 at 08:00 (before earliest) should be filtered out") + } + if _, ok := lectureGroups["02"]; !ok { + t.Error("class 02 at 10:00 (within range) should be kept") + } + if _, ok := lectureGroups["03"]; ok { + t.Error("class 03 at 17:00 (after latest) should be filtered out") + } +} + +func TestMergeAndFilterModuleSlots_RecordedLessonBypassesFilters(t *testing.T) { + venues := minVenues() + freeDays := map[string]struct{}{"Monday": {}} + recordings := map[string]struct{}{"CS1010S|Lecture": {}} + + // Lecture class on a free day should be kept because it's recorded + timetable := []models.ModuleSlot{ + makeRawSlot("Lecture", "01", "Monday", "0900", "1000", "BIZ2-0111"), + } + + merged, _ := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 480, 1020) + + if _, ok := merged["Lecture"]["01"]; !ok { + t.Error("recorded lecture on free day should not be filtered out") + } +} + +func TestMergeAndFilterModuleSlots_RecordedBypassesTimeRange(t *testing.T) { + venues := minVenues() + freeDays := map[string]struct{}{} + recordings := map[string]struct{}{"CS1010S|Lecture": {}} + + // Lecture outside time range but recorded -> should be kept + timetable := []models.ModuleSlot{ + makeRawSlot("Lecture", "01", "Tuesday", "0700", "0800", "BIZ2-0111"), + } + + merged, _ := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 480, 1020) + + if _, ok := merged["Lecture"]["01"]; !ok { + t.Error("recorded lecture outside time range should not be filtered out") + } +} + +func TestMergeAndFilterModuleSlots_VenueCoordinatesAssigned(t *testing.T) { + venues := minVenues() + freeDays := map[string]struct{}{} + recordings := map[string]struct{}{} + + timetable := []models.ModuleSlot{ + makeRawSlot("Lecture", "01", "Tuesday", "0900", "1000", "BIZ2-0111"), // in venues + makeRawSlot("Tutorial", "T01", "Wednesday", "1000", "1100", "UNKNOWN"), // not in venues + } + + merged, _ := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 0, 1440) + + lecSlots := merged["Lecture"]["01"] + if len(lecSlots) == 0 { + t.Fatal("expected lecture class 01 in merged") + } + if lecSlots[0].Coordinates != (models.Coordinates{X: 103.7748, Y: 1.2936}) { + t.Errorf("expected BIZ2-0111 coordinates, got %v", lecSlots[0].Coordinates) + } + + tutSlots := merged["Tutorial"]["T01"] + if len(tutSlots) == 0 { + t.Fatal("expected tutorial class T01 in merged") + } + if tutSlots[0].Coordinates != constants.InvalidCoordinates { + t.Errorf("expected InvalidCoordinates for unknown venue, got %v", tutSlots[0].Coordinates) + } +} + +func TestMergeAndFilterModuleSlots_VenueWithZeroCoordinatesGetsInvalid(t *testing.T) { + // A venue in the map but with X=0 or Y=0 should be treated as missing + venues := map[string]models.Location{ + "ZEROVENUE": {Location: models.Coordinates{X: 0, Y: 0}}, + } + freeDays := map[string]struct{}{} + recordings := map[string]struct{}{} + + timetable := []models.ModuleSlot{ + makeRawSlot("Lecture", "01", "Tuesday", "0900", "1000", "ZEROVENUE"), + } + + merged, _ := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 0, 1440) + + slots := merged["Lecture"]["01"] + if len(slots) == 0 { + t.Fatal("expected class 01 in merged") + } + if slots[0].Coordinates != constants.InvalidCoordinates { + t.Errorf("expected InvalidCoordinates for zero-coord venue, got %v", slots[0].Coordinates) + } +} + +func TestMergeAndFilterModuleSlots_DuplicateSlotsAreMerged(t *testing.T) { + // Two classes with same day/time/building/weeks -> only one should survive + venues := minVenues() + freeDays := map[string]struct{}{} + recordings := map[string]struct{}{} + + timetable := []models.ModuleSlot{ + { + LessonType: "Lecture", + ClassNo: "01", + Day: "Tuesday", + StartTime: "0900", + EndTime: "1000", + Venue: "BIZ2-0111", + WeeksString: "1,2,3", + }, + { + LessonType: "Lecture", + ClassNo: "02", + Day: "Tuesday", + StartTime: "0900", + EndTime: "1000", + Venue: "BIZ2-0222", + WeeksString: "1,2,3", + }, + } + + merged, _ := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 0, 1440) + + lectureGroups := merged["Lecture"] + if len(lectureGroups) != 1 { + t.Errorf("expected 1 unique Lecture class group after deduplication, got %d", len(lectureGroups)) + } +} + +func TestMergeAndFilterModuleSlots_EVenuesNotDeduplicated(t *testing.T) { + // E-Venue slots should never be deduplicated regardless of identical day/time + venues := minVenues() + freeDays := map[string]struct{}{} + recordings := map[string]struct{}{} + + timetable := []models.ModuleSlot{ + { + LessonType: "Lecture", + ClassNo: "E1", + Day: "Tuesday", + StartTime: "0900", + EndTime: "1000", + Venue: "E-Learn_A", + WeeksString: "1,2", + }, + { + LessonType: "Lecture", + ClassNo: "E2", + Day: "Tuesday", + StartTime: "0900", + EndTime: "1000", + Venue: "E-Learn_A", + WeeksString: "1,2", + }, + } + + merged, _ := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 0, 1440) + + lectureGroups := merged["Lecture"] + if len(lectureGroups) != 2 { + t.Errorf("expected 2 E-Venue class groups (no dedup), got %d: %v", len(lectureGroups), lectureGroups) + } +} + +func TestMergeAndFilterModuleSlots_DefaultSlotsPopulated(t *testing.T) { + // defaultSlots should contain a slot for every lesson type encountered + venues := minVenues() + freeDays := map[string]struct{}{} + recordings := map[string]struct{}{} + + timetable := []models.ModuleSlot{ + makeRawSlot("Lecture", "01", "Tuesday", "0900", "1000", "BIZ2-0111"), + makeRawSlot("Tutorial", "T01", "Wednesday", "1000", "1100", "COM1-0212"), + } + + _, defaultSlots := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 0, 1440) + + if _, ok := defaultSlots["Lecture"]; !ok { + t.Error("expected Lecture in defaultSlots") + } + if _, ok := defaultSlots["Tutorial"]; !ok { + t.Error("expected Tutorial in defaultSlots") + } +} + +func TestMergeAndFilterModuleSlots_PairedSlotsFilteredTogether(t *testing.T) { + // A class with two paired slots (e.g., two lectures per week) must have BOTH + // pass constraints — if one fails, the whole class is excluded. + venues := minVenues() + freeDays := map[string]struct{}{"Wednesday": {}} + recordings := map[string]struct{}{} + + // Class "01" has two paired slots: Tuesday (ok) + Wednesday (free day -> fails) + timetable := []models.ModuleSlot{ + makeRawSlot("Lecture", "01", "Tuesday", "0900", "1000", "BIZ2-0111"), + makeRawSlot("Lecture", "01", "Wednesday", "0900", "1000", "BIZ2-0111"), // free day + makeRawSlot("Lecture", "02", "Tuesday", "1100", "1200", "BIZ2-0111"), // no issues + } + + merged, _ := mergeAndFilterModuleSlots(timetable, venues, "CS1010S", recordings, freeDays, 0, 1440) + + if _, ok := merged["Lecture"]["01"]; ok { + t.Error("class 01 with one slot on free day should be entirely excluded") + } + if _, ok := merged["Lecture"]["02"]; !ok { + t.Error("class 02 with no issues should be kept") + } +} + +func TestMergeAndFilterModuleSlots_EmptyTimetable(t *testing.T) { + venues := minVenues() + freeDays := map[string]struct{}{} + recordings := map[string]struct{}{} + + merged, defaultSlots := mergeAndFilterModuleSlots( + nil, venues, "CS1010S", recordings, freeDays, 0, 1440, + ) + + if len(merged) != 0 { + t.Errorf("expected empty merged for nil timetable, got %v", merged) + } + if len(defaultSlots) != 0 { + t.Errorf("expected empty defaultSlots for nil timetable, got %v", defaultSlots) + } +} diff --git a/website/api/optimiser/_solver/nusmods_link.go b/website/api/optimiser/_solver/nusmods_link.go index ec2bea327b..b8d8e60dee 100644 --- a/website/api/optimiser/_solver/nusmods_link.go +++ b/website/api/optimiser/_solver/nusmods_link.go @@ -35,7 +35,7 @@ func FillDefaultsAndGenerateShareableLinks( defaultConfig := createConfig(assignments, lessonToSlots) defaultSerializedConfig := serializeConfig(defaultConfig) - semesterPath := "" + var semesterPath string switch req.AcadSem { case 1: semesterPath = "sem-1" @@ -61,7 +61,7 @@ func FillDefaultsAndGenerateShareableLinks( return shareableURL, defaultShareableURL } -// Parses the assignments into a map of module codes to lesson types to class numbers +// Parses the assignments into a map of module codes to lesson types to class numbers. func createConfig( assignments map[string]string, lessonToSlots map[string][][]models.ModuleSlot, @@ -98,7 +98,7 @@ func createConfig( return config } -// Constructs the URL +// Constructs the URL. func serializeConfig(config map[string]map[string][]models.LessonIndex) string { var moduleParams []string @@ -126,7 +126,7 @@ func serializeConfig(config map[string]map[string][]models.LessonIndex) string { // Serializes an array of lesson indices into the format used in timetable share links // -// Returns "1,2,3" with input [1, 2, 3] +// Returns "1,2,3" with input [1, 2, 3]. func serializeLessonIndices(lessonIndex []models.LessonIndex) string { parts := make([]string, len(lessonIndex)) for i, idx := range lessonIndex { diff --git a/website/api/optimiser/_solver/nusmods_link_test.go b/website/api/optimiser/_solver/nusmods_link_test.go new file mode 100644 index 0000000000..4b9fe98d95 --- /dev/null +++ b/website/api/optimiser/_solver/nusmods_link_test.go @@ -0,0 +1,314 @@ +package solver + +import ( + "strings" + "testing" + + constants "github.com/nusmodifications/nusmods/website/api/optimiser/_constants" + models "github.com/nusmodifications/nusmods/website/api/optimiser/_models" +) + +// ────────────────────────────────────────────────── +// serializeLessonIndices +// ────────────────────────────────────────────────── + +func TestSerializeLessonIndices(t *testing.T) { + tests := []struct { + name string + input []models.LessonIndex + want string + }{ + {"empty slice", nil, ""}, + {"empty non-nil slice", []models.LessonIndex{}, ""}, + {"single zero", []models.LessonIndex{0}, "0"}, + {"single non-zero", []models.LessonIndex{5}, "5"}, + {"multiple in order", []models.LessonIndex{1, 2, 3}, "1,2,3"}, + {"multiple out of order preserved", []models.LessonIndex{3, 1, 2}, "3,1,2"}, + {"large indices", []models.LessonIndex{100, 200, 300}, "100,200,300"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := serializeLessonIndices(tt.input) + if got != tt.want { + t.Errorf("serializeLessonIndices(%v) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + +// ────────────────────────────────────────────────── +// createConfig +// ────────────────────────────────────────────────── + +func TestCreateConfig(t *testing.T) { + t.Run("single assignment maps to lesson indices", func(t *testing.T) { + assignments := map[string]string{"CS1010S|Lecture": "01"} + lessonToSlots := map[string][][]models.ModuleSlot{ + "CS1010S|Lecture": { + { + {ClassNo: "01", LessonIndex: 0}, + {ClassNo: "01", LessonIndex: 2}, + }, + { + {ClassNo: "02", LessonIndex: 1}, + }, + }, + } + + config := createConfig(assignments, lessonToSlots) + + if config["CS1010S"] == nil { + t.Fatal("expected CS1010S in config") + } + indices := config["CS1010S"]["Lecture"] + if len(indices) != 2 { + t.Fatalf("expected 2 lesson indices, got %d: %v", len(indices), indices) + } + if indices[0] != 0 || indices[1] != 2 { + t.Errorf("expected indices [0 2], got %v", indices) + } + }) + + t.Run("assignment with classNo not in lessonToSlots groups produces no indices", func(t *testing.T) { + assignments := map[string]string{"CS1010S|Lecture": "99"} + lessonToSlots := map[string][][]models.ModuleSlot{ + "CS1010S|Lecture": { + {{ClassNo: "01", LessonIndex: 0}}, + }, + } + config := createConfig(assignments, lessonToSlots) + // classNo "99" is never found -> no indices appended + if indices := config["CS1010S"]["Lecture"]; len(indices) != 0 { + t.Errorf("expected empty indices for missing classNo, got %v", indices) + } + }) + + t.Run("malformed lesson key without pipe is skipped", func(t *testing.T) { + assignments := map[string]string{"NOPIPE": "01"} + lessonToSlots := map[string][][]models.ModuleSlot{} + config := createConfig(assignments, lessonToSlots) + if len(config) != 0 { + t.Errorf("expected empty config for malformed key, got %v", config) + } + }) + + t.Run("multiple module assignments", func(t *testing.T) { + assignments := map[string]string{ + "CS1010S|Lecture": "01", + "MA1521|Tutorial": "T01", + } + lessonToSlots := map[string][][]models.ModuleSlot{ + "CS1010S|Lecture": {{{ClassNo: "01", LessonIndex: 5}}}, + "MA1521|Tutorial": {{{ClassNo: "T01", LessonIndex: 3}}}, + } + config := createConfig(assignments, lessonToSlots) + + if len(config) != 2 { + t.Errorf("expected 2 modules in config, got %d", len(config)) + } + if config["CS1010S"]["Lecture"][0] != 5 { + t.Errorf("CS1010S Lecture index = %d, want 5", config["CS1010S"]["Lecture"][0]) + } + if config["MA1521"]["Tutorial"][0] != 3 { + t.Errorf("MA1521 Tutorial index = %d, want 3", config["MA1521"]["Tutorial"][0]) + } + }) + + t.Run("same module multiple lesson types", func(t *testing.T) { + assignments := map[string]string{ + "CS2040S|Lecture": "1", + "CS2040S|Tutorial": "T1", + } + lessonToSlots := map[string][][]models.ModuleSlot{ + "CS2040S|Lecture": {{{ClassNo: "1", LessonIndex: 0}}}, + "CS2040S|Tutorial": {{{ClassNo: "T1", LessonIndex: 10}}}, + } + config := createConfig(assignments, lessonToSlots) + if config["CS2040S"] == nil { + t.Fatal("expected CS2040S in config") + } + if len(config["CS2040S"]) != 2 { + t.Errorf("expected 2 lesson types for CS2040S, got %d", len(config["CS2040S"])) + } + }) +} + +// ────────────────────────────────────────────────── +// serializeConfig +// ────────────────────────────────────────────────── + +func TestSerializeConfig(t *testing.T) { + t.Run("empty config returns empty string", func(t *testing.T) { + got := serializeConfig(map[string]map[string][]models.LessonIndex{}) + if got != "" { + t.Errorf("expected empty string, got %q", got) + } + }) + + t.Run("single module single lecture", func(t *testing.T) { + config := map[string]map[string][]models.LessonIndex{ + "CS1010S": {"Lecture": {0, 2}}, + } + got := serializeConfig(config) + // LessonTypeAbbrev["LECTURE"] = "LEC" + if !strings.Contains(got, "CS1010S") { + t.Errorf("serialized config %q missing module code CS1010S", got) + } + if !strings.Contains(got, "LEC:") { + t.Errorf("serialized config %q missing lesson type abbreviation LEC", got) + } + if !strings.Contains(got, "(0,2)") { + t.Errorf("serialized config %q missing lesson indices (0,2)", got) + } + }) + + t.Run("single module multiple lesson types joined with separator", func(t *testing.T) { + config := map[string]map[string][]models.LessonIndex{ + "CS1010S": { + "Lecture": {0}, + "Tutorial": {5}, + }, + } + got := serializeConfig(config) + // Both LEC and TUT should appear, joined by ModuleCodeSeparator ";" + if !strings.Contains(got, "LEC:(0)") || !strings.Contains(got, "TUT:(5)") { + t.Errorf("expected both LEC:(0) and TUT:(5) in serialized output: %q", got) + } + if !strings.Contains(got, constants.ModuleCodeSeparator) { + t.Errorf("expected ModuleCodeSeparator %q in multi-lesson output: %q", + constants.ModuleCodeSeparator, got) + } + }) + + t.Run("multiple modules joined with ampersand", func(t *testing.T) { + config := map[string]map[string][]models.LessonIndex{ + "CS1010S": {"Lecture": {0}}, + "MA1521": {"Lecture": {3}}, + } + got := serializeConfig(config) + if !strings.Contains(got, "&") { + t.Errorf("expected & between modules in %q", got) + } + }) + + t.Run("unknown lesson type abbreviation is empty string not panic", func(t *testing.T) { + config := map[string]map[string][]models.LessonIndex{ + "CS1010S": {"UNKNOWN_TYPE": {0}}, + } + // Should not panic; abbrev will be empty string + got := serializeConfig(config) + if !strings.Contains(got, "CS1010S") { + t.Logf("got: %q", got) + } + // Just verify it doesn't panic and returns a string + _ = got + }) +} + +// ────────────────────────────────────────────────── +// FillDefaultsAndGenerateShareableLinks +// ────────────────────────────────────────────────── + +func TestFillDefaultsAndGenerateShareableLinks_SemesterPaths(t *testing.T) { + semTests := []struct { + acadSem int + wantPathSeg string + }{ + {1, "/sem-1/share"}, + {2, "/sem-2/share"}, + {3, "/st-i/share"}, + {4, "/st-ii/share"}, + {99, "/sem-1/share"}, // default case + } + + for _, tt := range semTests { + t.Run("sem_path", func(t *testing.T) { + assignments := map[string]string{"CS1010S|Lecture": "01"} + lessonToSlots := map[string][][]models.ModuleSlot{ + "CS1010S|Lecture": { + {{ClassNo: "01", LessonIndex: 0}}, + }, + } + defaultSlots := map[string]map[string][]models.ModuleSlot{} + req := models.OptimiserRequest{AcadSem: tt.acadSem} + + link, _ := FillDefaultsAndGenerateShareableLinks(assignments, defaultSlots, lessonToSlots, req) + + if !strings.Contains(link, tt.wantPathSeg) { + t.Errorf("AcadSem=%d: URL %q does not contain %q", tt.acadSem, link, tt.wantPathSeg) + } + if !strings.HasPrefix(link, constants.NUSModsTimetableBaseURL) { + t.Errorf("URL %q should start with %q", link, constants.NUSModsTimetableBaseURL) + } + }) + } +} + +func TestFillDefaultsAndGenerateShareableLinks_DefaultFilling(t *testing.T) { + t.Run("unassigned lesson gets default class from defaultSlots", func(t *testing.T) { + assignments := map[string]string{} // nothing assigned + defaultSlots := map[string]map[string][]models.ModuleSlot{ + "cs1010s": { + "Tutorial": { + {ClassNo: "T01", LessonIndex: 7}, + }, + }, + } + lessonToSlots := map[string][][]models.ModuleSlot{} + req := models.OptimiserRequest{AcadSem: 1} + + _, defaultLink := FillDefaultsAndGenerateShareableLinks(assignments, defaultSlots, lessonToSlots, req) + + // After the call, assignments["CS1010S|Tutorial"] should be "T01" + if assignments["CS1010S|Tutorial"] != "T01" { + t.Errorf("expected default assignment CS1010S|Tutorial=T01, got %q", assignments["CS1010S|Tutorial"]) + } + // defaultLink should contain the module code + if !strings.Contains(defaultLink, "CS1010S") { + t.Errorf("defaultLink %q does not contain CS1010S", defaultLink) + } + }) + + t.Run("already-assigned lesson is not overwritten by default", func(t *testing.T) { + assignments := map[string]string{"CS1010S|Lecture": "02"} + defaultSlots := map[string]map[string][]models.ModuleSlot{ + "cs1010s": { + "Lecture": { + {ClassNo: "01", LessonIndex: 0}, + }, + }, + } + lessonToSlots := map[string][][]models.ModuleSlot{ + "CS1010S|Lecture": { + {{ClassNo: "02", LessonIndex: 1}}, + {{ClassNo: "01", LessonIndex: 0}}, + }, + } + req := models.OptimiserRequest{AcadSem: 1} + + FillDefaultsAndGenerateShareableLinks(assignments, defaultSlots, lessonToSlots, req) + + if assignments["CS1010S|Lecture"] != "02" { + t.Errorf("expected assignment preserved as 02, got %q", assignments["CS1010S|Lecture"]) + } + }) + + t.Run("two links returned for same academic semester", func(t *testing.T) { + assignments := map[string]string{"CS1010S|Lecture": "01"} + lessonToSlots := map[string][][]models.ModuleSlot{ + "CS1010S|Lecture": {{{ClassNo: "01", LessonIndex: 0}}}, + } + defaultSlots := map[string]map[string][]models.ModuleSlot{} + req := models.OptimiserRequest{AcadSem: 2} + + link, defaultLink := FillDefaultsAndGenerateShareableLinks(assignments, defaultSlots, lessonToSlots, req) + + if !strings.Contains(link, "/sem-2/share") { + t.Errorf("link %q missing /sem-2/share", link) + } + if !strings.Contains(defaultLink, "/sem-2/share") { + t.Errorf("defaultLink %q missing /sem-2/share", defaultLink) + } + }) +} diff --git a/website/api/optimiser/_solver/solver.go b/website/api/optimiser/_solver/solver.go index 70f6e2a69b..bbc5721187 100644 --- a/website/api/optimiser/_solver/solver.go +++ b/website/api/optimiser/_solver/solver.go @@ -1,6 +1,7 @@ package solver import ( + "maps" "net/http" "sort" "strings" @@ -96,11 +97,10 @@ func beamSearch( branchingFactor int, recordings map[string]struct{}, optimiserRequest models.OptimiserRequest) models.TimetableState { - initial := models.TimetableState{ Assignments: make(map[string]string), } - for d := 0; d < constants.DaysPerWeek; d++ { + for d := range constants.DaysPerWeek { initial.DaySlots[d] = make([]models.ModuleSlot, 0) } beam := []models.TimetableState{initial} @@ -112,7 +112,7 @@ func beamSearch( // Filter valid slot groups validGroups := make([][]models.ModuleSlot, 0, limit) - for i := 0; i < limit; i++ { + for i := range limit { group := slotGroups[i] validGroup := make([]models.ModuleSlot, 0, len(group)) for slotIdx := range group { @@ -128,7 +128,6 @@ func beamSearch( // iterate over all partial timetables in the beam for _, state := range beam { - // iterate over all pre-filtered slot groups for the current lesson for _, validGroup := range validGroups { if hasConflict(state, validGroup) { @@ -189,7 +188,6 @@ func hasConflict(state models.TimetableState, newSlots []models.ModuleSlot) bool for _, oldSlot := range state.DaySlots[newSlot.DayIndex] { // Check if slots overlap in time if newSlot.StartMin < oldSlot.EndMin && oldSlot.StartMin < newSlot.EndMin { - // if weeks is not a []int, then skip checking for week conflict if newSlot.WeeksSet == nil || oldSlot.WeeksSet == nil { return true @@ -218,12 +216,10 @@ func copyState(src models.TimetableState) models.TimetableState { } // Copy assignments - for k, v := range src.Assignments { - newState.Assignments[k] = v - } + maps.Copy(newState.Assignments, src.Assignments) // Copy day slots - for i := 0; i < constants.DaysPerWeek; i++ { + for i := range constants.DaysPerWeek { if len(src.DaySlots[i]) > 0 { newState.DaySlots[i] = make([]models.ModuleSlot, len(src.DaySlots[i])) copy(newState.DaySlots[i], src.DaySlots[i]) @@ -309,7 +305,7 @@ func isLessonRecorded(lessonKey string, recordings map[string]struct{}) bool { return ok } -// isInvalidCoordinates checks if coordinates passed are valid +// isInvalidCoordinates checks if coordinates passed are valid. func isInvalidCoordinates(coord models.Coordinates) bool { return coord == constants.InvalidCoordinates } @@ -328,7 +324,7 @@ func scoreTimetableState( optimiserRequest models.OptimiserRequest, ) float64 { var totalScore float64 - for d := 0; d < constants.DaysPerWeek; d++ { + for d := range constants.DaysPerWeek { if len(state.DaySlots[d]) == 0 { continue } @@ -457,7 +453,7 @@ func scoreConsecutiveHoursOfStudy(physicalSlots []models.ModuleSlot, maxConsecut score := 0 consecutiveMinutes := 0 - for i := 0; i < len(physicalSlots); i++ { + for i := range physicalSlots { currentSlot := physicalSlots[i] currentSlotStartMin := currentSlot.StartMin currentSlotEndMin := currentSlot.EndMin diff --git a/website/api/optimiser/_solver/solver_test.go b/website/api/optimiser/_solver/solver_test.go new file mode 100644 index 0000000000..e5ceac8abc --- /dev/null +++ b/website/api/optimiser/_solver/solver_test.go @@ -0,0 +1,863 @@ +package solver + +import ( + "math" + "testing" + + constants "github.com/nusmodifications/nusmods/website/api/optimiser/_constants" + models "github.com/nusmodifications/nusmods/website/api/optimiser/_models" +) + +// makeSlot builds a ModuleSlot with parsed time/day fields populated via ParseModuleSlotFields. +func makeSlot(lessonKey, day, startTime, endTime string) models.ModuleSlot { + slot := models.ModuleSlot{ + Day: day, + StartTime: startTime, + EndTime: endTime, + Coordinates: constants.InvalidCoordinates, + } + _ = slot.ParseModuleSlotFields(lessonKey) + return slot +} + +// makeSlotWithCoords creates a slot with specific venue coordinates. +func makeSlotWithCoords(lessonKey, day, startTime, endTime string, coords models.Coordinates) models.ModuleSlot { + slot := makeSlot(lessonKey, day, startTime, endTime) + slot.Coordinates = coords + return slot +} + +// makeSlotWithWeeks creates a slot with a specific week set. +func makeSlotWithWeeks(lessonKey, day, startTime, endTime string, weeks []int) models.ModuleSlot { + slot := makeSlot(lessonKey, day, startTime, endTime) + slot.WeeksSet = make(map[int]struct{}, len(weeks)) + for _, w := range weeks { + slot.WeeksSet[w] = struct{}{} + } + return slot +} + +// lunchReq returns an OptimiserRequest with lunch window set. +func lunchReq(lunchStart, lunchEnd, maxHours int) models.OptimiserRequest { + return models.OptimiserRequest{ + LunchStartMin: lunchStart, + LunchEndMin: lunchEnd, + MaxConsecutiveHours: maxHours, + } +} + +// stateWithDay returns a TimetableState with the given slots on day 0 (Monday). +func stateWithDay(slots []models.ModuleSlot) models.TimetableState { + s := models.TimetableState{ + Assignments: make(map[string]string), + } + for d := range s.DaySlots { + s.DaySlots[d] = make([]models.ModuleSlot, 0) + } + s.DaySlots[0] = slots + return s +} + +// ────────────────────────────────────────────────── +// hasConflict +// ────────────────────────────────────────────────── + +func TestHasConflict(t *testing.T) { + t.Run("empty state has no conflict", func(t *testing.T) { + state := stateWithDay(nil) + newSlot := makeSlot("A|Lecture", "Monday", "0900", "1000") + if hasConflict(state, []models.ModuleSlot{newSlot}) { + t.Error("expected no conflict against empty state") + } + }) + + t.Run("different days never conflict", func(t *testing.T) { + existing := makeSlot("A|Lecture", "Monday", "0900", "1100") + state := stateWithDay([]models.ModuleSlot{existing}) + newSlot := makeSlot("B|Lecture", "Tuesday", "0900", "1100") + if hasConflict(state, []models.ModuleSlot{newSlot}) { + t.Error("expected no conflict for different days") + } + }) + + t.Run("same day non-overlapping times no conflict", func(t *testing.T) { + existing := makeSlot("A|Lecture", "Monday", "0900", "1000") + state := stateWithDay([]models.ModuleSlot{existing}) + // starts exactly when existing ends — adjacent, not overlapping + newSlot := makeSlot("B|Lecture", "Monday", "1000", "1100") + if hasConflict(state, []models.ModuleSlot{newSlot}) { + t.Error("adjacent slots should not conflict (open interval overlap)") + } + }) + + t.Run("same day non-overlapping with gap no conflict", func(t *testing.T) { + existing := makeSlot("A|Lecture", "Monday", "0900", "1000") + state := stateWithDay([]models.ModuleSlot{existing}) + newSlot := makeSlot("B|Lecture", "Monday", "1100", "1200") + if hasConflict(state, []models.ModuleSlot{newSlot}) { + t.Error("expected no conflict for distinct time slots") + } + }) + + t.Run("same day overlapping times both nil WeeksSet returns conflict", func(t *testing.T) { + existing := makeSlot("A|Lecture", "Monday", "0900", "1100") + // WeeksSet is nil by default from makeSlot + state := stateWithDay([]models.ModuleSlot{existing}) + newSlot := makeSlot("B|Lecture", "Monday", "1000", "1200") + if !hasConflict(state, []models.ModuleSlot{newSlot}) { + t.Error("expected conflict for overlapping times with nil weeks") + } + }) + + t.Run("same day overlapping times overlapping weeks returns conflict", func(t *testing.T) { + existing := makeSlotWithWeeks("A|Lecture", "Monday", "0900", "1100", []int{1, 2, 3}) + state := stateWithDay([]models.ModuleSlot{existing}) + newSlot := makeSlotWithWeeks("B|Lecture", "Monday", "1000", "1200", []int{3, 4, 5}) + if !hasConflict(state, []models.ModuleSlot{newSlot}) { + t.Error("expected conflict for overlapping times and overlapping weeks") + } + }) + + t.Run("same day overlapping times non-overlapping weeks no conflict", func(t *testing.T) { + existing := makeSlotWithWeeks("A|Lecture", "Monday", "0900", "1100", []int{1, 2, 3}) + state := stateWithDay([]models.ModuleSlot{existing}) + newSlot := makeSlotWithWeeks("B|Lecture", "Monday", "0900", "1100", []int{4, 5, 6}) + if hasConflict(state, []models.ModuleSlot{newSlot}) { + t.Error("expected no conflict for overlapping times but distinct weeks") + } + }) + + t.Run("newSlot fully inside existing slot is conflict", func(t *testing.T) { + existing := makeSlot("A|Lecture", "Monday", "0900", "1200") + state := stateWithDay([]models.ModuleSlot{existing}) + newSlot := makeSlot("B|Lecture", "Monday", "1000", "1100") + if !hasConflict(state, []models.ModuleSlot{newSlot}) { + t.Error("expected conflict for nested time slots") + } + }) + + t.Run("multiple new slots one conflicts", func(t *testing.T) { + existing := makeSlot("A|Lecture", "Monday", "0900", "1000") + state := stateWithDay([]models.ModuleSlot{existing}) + noConflict := makeSlot("B|Tutorial", "Tuesday", "1000", "1100") + conflict := makeSlot("C|Lab", "Monday", "0930", "1030") + if !hasConflict(state, []models.ModuleSlot{noConflict, conflict}) { + t.Error("expected conflict when at least one new slot conflicts") + } + }) + + t.Run("one week set nil the other non-nil overlapping time returns conflict", func(t *testing.T) { + existing := makeSlot("A|Lecture", "Monday", "0900", "1100") // WeeksSet nil + state := stateWithDay([]models.ModuleSlot{existing}) + newSlot := makeSlotWithWeeks("B|Lecture", "Monday", "1000", "1200", []int{1, 2}) + if !hasConflict(state, []models.ModuleSlot{newSlot}) { + t.Error("expected conflict when existing has nil WeeksSet and times overlap") + } + }) +} + +// ────────────────────────────────────────────────── +// copyState +// ────────────────────────────────────────────────── + +func TestCopyState(t *testing.T) { + t.Run("copy of empty state is independent", func(t *testing.T) { + src := models.TimetableState{ + Assignments: make(map[string]string), + } + for d := range src.DaySlots { + src.DaySlots[d] = make([]models.ModuleSlot, 0) + } + dst := copyState(src) + dst.Assignments["X|Y"] = "01" + if _, ok := src.Assignments["X|Y"]; ok { + t.Error("modifying copy's Assignments should not affect original") + } + }) + + t.Run("modifying copy Assignments does not affect original", func(t *testing.T) { + src := models.TimetableState{ + Assignments: map[string]string{"CS1010S|Lecture": "01"}, + } + for d := range src.DaySlots { + src.DaySlots[d] = make([]models.ModuleSlot, 0) + } + dst := copyState(src) + dst.Assignments["CS1010S|Lecture"] = "99" + if src.Assignments["CS1010S|Lecture"] != "01" { + t.Error("original Assignments should not be mutated by copy modification") + } + }) + + t.Run("modifying copy DaySlots does not affect original", func(t *testing.T) { + slot := makeSlot("A|Lecture", "Monday", "0900", "1000") + src := stateWithDay([]models.ModuleSlot{slot}) + initialLen := len(src.DaySlots[0]) + + dst := copyState(src) + extraSlot := makeSlot("B|Tutorial", "Monday", "1000", "1100") + dst.DaySlots[0] = append(dst.DaySlots[0], extraSlot) + + if len(src.DaySlots[0]) != initialLen { + t.Error("modifying copy's DaySlots should not change original length") + } + }) + + t.Run("distance values are copied", func(t *testing.T) { + src := models.TimetableState{ + Assignments: make(map[string]string), + TotalDistance: 42.5, + DayDistance: [6]float64{1, 2, 3, 4, 5, 6}, + } + for d := range src.DaySlots { + src.DaySlots[d] = make([]models.ModuleSlot, 0) + } + dst := copyState(src) + if dst.TotalDistance != 42.5 { + t.Errorf("TotalDistance = %v, want 42.5", dst.TotalDistance) + } + if dst.DayDistance != src.DayDistance { + t.Errorf("DayDistance mismatch: got %v want %v", dst.DayDistance, src.DayDistance) + } + }) + + t.Run("modifying copy distance does not affect original", func(t *testing.T) { + src := models.TimetableState{ + Assignments: make(map[string]string), + TotalDistance: 10.0, + } + for d := range src.DaySlots { + src.DaySlots[d] = make([]models.ModuleSlot, 0) + } + dst := copyState(src) + dst.TotalDistance = 99.0 + if src.TotalDistance != 10.0 { + t.Error("original TotalDistance should not be affected by copy modification") + } + }) +} + +// ────────────────────────────────────────────────── +// insertSlotSorted +// ────────────────────────────────────────────────── + +func TestInsertSlotSorted(t *testing.T) { + slot := func(startMin int) models.ModuleSlot { + return models.ModuleSlot{StartMin: startMin} + } + + assertOrder := func(t *testing.T, slots []models.ModuleSlot, wantMins []int) { + t.Helper() + if len(slots) != len(wantMins) { + t.Fatalf("len(slots)=%d want %d", len(slots), len(wantMins)) + } + for i, s := range slots { + if s.StartMin != wantMins[i] { + t.Errorf("slots[%d].StartMin = %d, want %d", i, s.StartMin, wantMins[i]) + } + } + } + + t.Run("insert into empty slice", func(t *testing.T) { + result := insertSlotSorted([]models.ModuleSlot{}, slot(540)) + assertOrder(t, result, []int{540}) + }) + + t.Run("insert smallest prepends", func(t *testing.T) { + existing := []models.ModuleSlot{slot(600), slot(720)} + result := insertSlotSorted(existing, slot(480)) + assertOrder(t, result, []int{480, 600, 720}) + }) + + t.Run("insert largest appends", func(t *testing.T) { + existing := []models.ModuleSlot{slot(480), slot(600)} + result := insertSlotSorted(existing, slot(720)) + assertOrder(t, result, []int{480, 600, 720}) + }) + + t.Run("insert in middle", func(t *testing.T) { + existing := []models.ModuleSlot{slot(480), slot(720)} + result := insertSlotSorted(existing, slot(600)) + assertOrder(t, result, []int{480, 600, 720}) + }) + + t.Run("insert with equal StartMin goes after", func(t *testing.T) { + // Binary search uses <=, so equal StartMin puts the new slot after the existing one. + existing := []models.ModuleSlot{slot(480), slot(600)} + result := insertSlotSorted(existing, slot(600)) + if len(result) != 3 { + t.Fatalf("expected 3 slots, got %d", len(result)) + } + if result[1].StartMin != 600 || result[2].StartMin != 600 { + t.Errorf("equal-StartMin slot not in positions 1,2: %v", result) + } + }) + + t.Run("insert into many slots stays sorted", func(t *testing.T) { + existing := []models.ModuleSlot{slot(480), slot(540), slot(660), slot(780)} + result := insertSlotSorted(existing, slot(600)) + assertOrder(t, result, []int{480, 540, 600, 660, 780}) + }) +} + +// ────────────────────────────────────────────────── +// isLessonRecorded +// ────────────────────────────────────────────────── + +func TestIsLessonRecorded(t *testing.T) { + recordings := map[string]struct{}{ + "CS1010S|Lecture": {}, + "CS2040S|Tutorial": {}, + } + + if !isLessonRecorded("CS1010S|Lecture", recordings) { + t.Error("expected CS1010S|Lecture to be recorded") + } + if !isLessonRecorded("CS2040S|Tutorial", recordings) { + t.Error("expected CS2040S|Tutorial to be recorded") + } + if isLessonRecorded("CS1010S|Tutorial", recordings) { + t.Error("CS1010S|Tutorial should not be recorded") + } + if isLessonRecorded("", recordings) { + t.Error("empty key should not be recorded") + } + if isLessonRecorded("CS1010S|Lecture", nil) { + t.Error("should not be recorded when recordings map is nil") + } + if isLessonRecorded("CS1010S|Lecture", map[string]struct{}{}) { + t.Error("should not be recorded when recordings map is empty") + } +} + +// ────────────────────────────────────────────────── +// isInvalidCoordinates +// ────────────────────────────────────────────────── + +func TestIsInvalidCoordinates(t *testing.T) { + t.Run("sentinel InvalidCoordinates returns true", func(t *testing.T) { + if !isInvalidCoordinates(constants.InvalidCoordinates) { + t.Error("expected InvalidCoordinates to be invalid") + } + }) + + t.Run("negative sentinel -1,-1 returns true", func(t *testing.T) { + if !isInvalidCoordinates(models.Coordinates{X: -1, Y: -1}) { + t.Error("expected {-1,-1} to be invalid") + } + }) + + t.Run("valid coordinates return false", func(t *testing.T) { + if isInvalidCoordinates(models.Coordinates{X: 103.7748, Y: 1.2936}) { + t.Error("expected real coordinates to be valid") + } + }) + + t.Run("zero coordinates are NOT the sentinel", func(t *testing.T) { + // Sentinel is {-1,-1}, not {0,0} + if isInvalidCoordinates(models.Coordinates{X: 0, Y: 0}) { + t.Error("{0,0} should be valid (not the InvalidCoordinates sentinel)") + } + }) + + t.Run("partial match not invalid", func(t *testing.T) { + if isInvalidCoordinates(models.Coordinates{X: -1, Y: 0}) { + t.Error("{-1,0} should be valid (partial match of sentinel)") + } + }) +} + +// ────────────────────────────────────────────────── +// calculateDayDistanceScore +// ────────────────────────────────────────────────── + +func TestCalculateDayDistanceScore(t *testing.T) { + noRecordings := map[string]struct{}{} + + t.Run("empty slots returns 0", func(t *testing.T) { + score := calculateDayDistanceScore(nil, noRecordings) + if score != 0 { + t.Errorf("got %v, want 0", score) + } + }) + + t.Run("single slot returns 0", func(t *testing.T) { + slot := makeSlotWithCoords("A|Lecture", "Monday", "0900", "1000", + models.Coordinates{X: 103.77, Y: 1.29}) + score := calculateDayDistanceScore([]models.ModuleSlot{slot}, noRecordings) + if score != 0 { + t.Errorf("got %v, want 0 for single slot", score) + } + }) + + t.Run("both slots recorded skips pair penalty", func(t *testing.T) { + recordings := map[string]struct{}{"A|Lecture": {}, "B|Tutorial": {}} + s1 := makeSlotWithCoords("A|Lecture", "Monday", "0900", "1000", constants.InvalidCoordinates) + s2 := makeSlotWithCoords("B|Tutorial", "Monday", "1000", "1100", constants.InvalidCoordinates) + score := calculateDayDistanceScore([]models.ModuleSlot{s1, s2}, recordings) + if score != 0 { + t.Errorf("got %v, want 0 when both slots are recorded", score) + } + }) + + t.Run("first slot recorded skips pair", func(t *testing.T) { + recordings := map[string]struct{}{"A|Lecture": {}} + s1 := makeSlotWithCoords("A|Lecture", "Monday", "0900", "1000", constants.InvalidCoordinates) + s2 := makeSlotWithCoords("B|Tutorial", "Monday", "1000", "1100", constants.InvalidCoordinates) + score := calculateDayDistanceScore([]models.ModuleSlot{s1, s2}, recordings) + if score != 0 { + t.Errorf("got %v, want 0 when first slot is recorded", score) + } + }) + + t.Run("both slots with invalid coords adds NoVenuePenalty", func(t *testing.T) { + s1 := makeSlot("A|Lecture", "Monday", "0900", "1000") // has InvalidCoordinates from makeSlot + s2 := makeSlot("B|Tutorial", "Monday", "1000", "1100") // has InvalidCoordinates from makeSlot + score := calculateDayDistanceScore([]models.ModuleSlot{s1, s2}, noRecordings) + if score != constants.NoVenuePenalty { + t.Errorf("got %v, want %v (NoVenuePenalty)", score, constants.NoVenuePenalty) + } + }) + + t.Run("second slot with invalid coords adds NoVenuePenalty", func(t *testing.T) { + s1 := makeSlotWithCoords("A|Lecture", "Monday", "0900", "1000", + models.Coordinates{X: 103.77, Y: 1.29}) + s2 := makeSlot("B|Tutorial", "Monday", "1000", "1100") // InvalidCoordinates + score := calculateDayDistanceScore([]models.ModuleSlot{s1, s2}, noRecordings) + if score != constants.NoVenuePenalty { + t.Errorf("got %v, want %v when second slot has invalid coords", score, constants.NoVenuePenalty) + } + }) + + t.Run("same valid coords gives near-zero penalty", func(t *testing.T) { + coord := models.Coordinates{X: 103.7748, Y: 1.2936} + s1 := makeSlotWithCoords("A|Lecture", "Monday", "0900", "1000", coord) + s2 := makeSlotWithCoords("B|Tutorial", "Monday", "1000", "1100", coord) + score := calculateDayDistanceScore([]models.ModuleSlot{s1, s2}, noRecordings) + if score > 0.001 { + t.Errorf("same-venue distance penalty = %v, want ~0", score) + } + }) + + t.Run("far-apart valid coords gives positive penalty", func(t *testing.T) { + // COM1 area vs BIZ area — roughly 400m apart + s1 := makeSlotWithCoords("A|Lecture", "Monday", "0900", "1000", + models.Coordinates{X: 103.7716, Y: 1.2952}) + s2 := makeSlotWithCoords("B|Tutorial", "Monday", "1000", "1100", + models.Coordinates{X: 103.7748, Y: 1.2936}) + score := calculateDayDistanceScore([]models.ModuleSlot{s1, s2}, noRecordings) + if score <= 0 { + t.Errorf("expected positive penalty for far-apart venues, got %v", score) + } + }) + + t.Run("three slots accumulates penalty over all pairs", func(t *testing.T) { + s1 := makeSlot("A|Lecture", "Monday", "0900", "1000") // InvalidCoords + s2 := makeSlot("B|Tutorial", "Monday", "1000", "1100") // InvalidCoords + s3 := makeSlot("C|Lab", "Monday", "1100", "1200") // InvalidCoords + score := calculateDayDistanceScore([]models.ModuleSlot{s1, s2, s3}, noRecordings) + if score != 2*constants.NoVenuePenalty { + t.Errorf("got %v, want %v (2 pairs × NoVenuePenalty)", score, 2*constants.NoVenuePenalty) + } + }) +} + +// ────────────────────────────────────────────────── +// getPhysicalSlots +// ────────────────────────────────────────────────── + +func TestGetPhysicalSlots(t *testing.T) { + s1 := makeSlot("A|Lecture", "Monday", "0900", "1000") + s2 := makeSlot("B|Tutorial", "Monday", "1000", "1100") + s3 := makeSlot("C|Lab", "Monday", "1100", "1200") + + t.Run("no recordings returns original slice", func(t *testing.T) { + result := getPhysicalSlots([]models.ModuleSlot{s1, s2}, map[string]struct{}{}) + if len(result) != 2 { + t.Errorf("got %d slots, want 2", len(result)) + } + }) + + t.Run("empty daySlots returns empty", func(t *testing.T) { + result := getPhysicalSlots([]models.ModuleSlot{}, map[string]struct{}{"A|Lecture": {}}) + if len(result) != 0 { + t.Errorf("got %d slots, want 0", len(result)) + } + }) + + t.Run("all recorded returns empty", func(t *testing.T) { + recordings := map[string]struct{}{"A|Lecture": {}, "B|Tutorial": {}} + result := getPhysicalSlots([]models.ModuleSlot{s1, s2}, recordings) + if len(result) != 0 { + t.Errorf("got %d slots, want 0 when all are recorded", len(result)) + } + }) + + t.Run("mixed returns only physical", func(t *testing.T) { + recordings := map[string]struct{}{"B|Tutorial": {}} + result := getPhysicalSlots([]models.ModuleSlot{s1, s2, s3}, recordings) + if len(result) != 2 { + t.Errorf("got %d slots, want 2", len(result)) + } + for _, s := range result { + if s.LessonKey == "B|Tutorial" { + t.Error("recorded slot should not appear in physical slots") + } + } + }) + + t.Run("nil recordings returns original", func(t *testing.T) { + // nil recordings means len(recordings)==0, returns daySlots as-is + result := getPhysicalSlots([]models.ModuleSlot{s1, s2}, nil) + if len(result) != 2 { + t.Errorf("got %d slots, want 2 for nil recordings", len(result)) + } + }) +} + +// ────────────────────────────────────────────────── +// calculateLunchGap (lunchStart=720=12:00, lunchEnd=840=14:00) +// ────────────────────────────────────────────────── + +func TestCalculateLunchGap(t *testing.T) { + req := lunchReq(720, 840, 4) // 12:00–14:00 lunch window + + t.Run("empty slots returns LunchRequiredTime", func(t *testing.T) { + gap := calculateLunchGap(nil, req) + if gap != constants.LunchRequiredTime { + t.Errorf("got %d, want %d", gap, constants.LunchRequiredTime) + } + }) + + t.Run("single morning class leaves full lunch window", func(t *testing.T) { + // Class 08:00–09:00, nothing during 12:00–14:00 + slot := models.ModuleSlot{StartMin: 480, EndMin: 540} + gap := calculateLunchGap([]models.ModuleSlot{slot}, req) + // gap after last class: 840 - max(540, 720) = 840 - 720 = 120 + if gap != 120 { + t.Errorf("got %d, want 120", gap) + } + }) + + t.Run("single class ending during lunch window", func(t *testing.T) { + // Class 11:00–13:00 (660–780), lunch start at 12:00 + slot := models.ModuleSlot{StartMin: 660, EndMin: 780} + gap := calculateLunchGap([]models.ModuleSlot{slot}, req) + // gap after: 840 - max(780, 720) = 840 - 780 = 60 + if gap != 60 { + t.Errorf("got %d, want 60", gap) + } + }) + + t.Run("single class spanning entire lunch window", func(t *testing.T) { + // Class 12:00–14:00 (720–840) + slot := models.ModuleSlot{StartMin: 720, EndMin: 840} + gap := calculateLunchGap([]models.ModuleSlot{slot}, req) + if gap != 0 { + t.Errorf("got %d, want 0 when class spans entire lunch window", gap) + } + }) + + t.Run("single afternoon class leaves full lunch window via gap-before", func(t *testing.T) { + // Class 14:00–15:00 (840–900), starts exactly at lunchEnd + slot := models.ModuleSlot{StartMin: 840, EndMin: 900} + // gap before: 840 > 720? yes. gap = min(840,840)-720 = 120. + gap := calculateLunchGap([]models.ModuleSlot{slot}, req) + if gap != 120 { + t.Errorf("got %d, want 120", gap) + } + }) + + t.Run("gap between two classes inside lunch window", func(t *testing.T) { + // Class1: 09:00–12:00 (540–720), Class2: 13:00–15:00 (780–900) + // Between gap: gapStart=max(720,720)=720, gapEnd=min(780,840)=780, gap=60 + s1 := models.ModuleSlot{StartMin: 540, EndMin: 720} + s2 := models.ModuleSlot{StartMin: 780, EndMin: 900} + gap := calculateLunchGap([]models.ModuleSlot{s1, s2}, req) + if gap != 60 { + t.Errorf("got %d, want 60", gap) + } + }) + + t.Run("two classes with gap spanning entire lunch window", func(t *testing.T) { + // Class1: 11:00–12:00 (660–720), Class2: 14:00–15:00 (840–900) + // Between gap: gapStart=max(720,720)=720, gapEnd=min(840,840)=840, gap=120 + s1 := models.ModuleSlot{StartMin: 660, EndMin: 720} + s2 := models.ModuleSlot{StartMin: 840, EndMin: 900} + gap := calculateLunchGap([]models.ModuleSlot{s1, s2}, req) + if gap != 120 { + t.Errorf("got %d, want 120", gap) + } + }) + + t.Run("back-to-back classes covering lunch gives zero gap", func(t *testing.T) { + // Class1: 11:00–12:00 (660–720), Class2: 12:00–14:00 (720–840) + s1 := models.ModuleSlot{StartMin: 660, EndMin: 720} + s2 := models.ModuleSlot{StartMin: 720, EndMin: 840} + gap := calculateLunchGap([]models.ModuleSlot{s1, s2}, req) + if gap != 0 { + t.Errorf("got %d, want 0 for back-to-back classes covering lunch", gap) + } + }) +} + +// ────────────────────────────────────────────────── +// calculateLargestGap +// ────────────────────────────────────────────────── + +func TestCalculateLargestGap(t *testing.T) { + tests := []struct { + name string + slots []models.ModuleSlot + wantGap int + }{ + { + "empty slots", + nil, + 0, + }, + { + "single slot", + []models.ModuleSlot{{StartMin: 540, EndMin: 600}}, + 0, + }, + { + "two adjacent slots", + []models.ModuleSlot{{StartMin: 540, EndMin: 600}, {StartMin: 600, EndMin: 660}}, + 0, + }, + { + "two slots with 90 min gap", + []models.ModuleSlot{{StartMin: 540, EndMin: 600}, {StartMin: 690, EndMin: 750}}, + 90, + }, + { + "three slots picks largest gap", + []models.ModuleSlot{ + {StartMin: 540, EndMin: 600}, // 09:00–10:00 + {StartMin: 660, EndMin: 720}, // 11:00–12:00, gap=60 + {StartMin: 900, EndMin: 960}, // 15:00–16:00, gap=180 + }, + 180, + }, + { + "large gap between first and second", + []models.ModuleSlot{ + {StartMin: 480, EndMin: 540}, // 08:00–09:00 + {StartMin: 900, EndMin: 960}, // 15:00–16:00, gap=360 + {StartMin: 960, EndMin: 1020}, // 16:00–17:00, gap=0 + }, + 360, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := calculateLargestGap(tt.slots) + if got != tt.wantGap { + t.Errorf("calculateLargestGap() = %d, want %d", got, tt.wantGap) + } + }) + } +} + +// ────────────────────────────────────────────────── +// penaliseConsecutiveHoursOfStudy +// ────────────────────────────────────────────────── + +func TestPenaliseConsecutiveHoursOfStudy(t *testing.T) { + tests := []struct { + name string + consecutiveMinutes int + maxHours int + want int + }{ + {"zero minutes no penalty", 0, 4, 0}, + {"1 hour under 4h limit", 60, 4, 0}, + {"exactly 4h at limit", 240, 4, 0}, + {"4h 1min: integer division rounds down to 4h, no penalty", 241, 4, 0}, + {"exactly 5h: 1h over", 300, 4, constants.ConsecutiveHoursPenaltyRate}, + {"exactly 6h: 2h over", 360, 4, 2 * constants.ConsecutiveHoursPenaltyRate}, + {"8h: 4h over", 480, 4, 4 * constants.ConsecutiveHoursPenaltyRate}, + {"2h with max=2 at limit", 120, 2, 0}, + {"3h with max=2: 1h over", 180, 2, constants.ConsecutiveHoursPenaltyRate}, + {"max=0 every class penalised", 60, 0, constants.ConsecutiveHoursPenaltyRate}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := penaliseConsecutiveHoursOfStudy(tt.consecutiveMinutes, tt.maxHours) + if got != tt.want { + t.Errorf("penaliseConsecutiveHoursOfStudy(%d, %d) = %d, want %d", + tt.consecutiveMinutes, tt.maxHours, got, tt.want) + } + }) + } +} + +// ────────────────────────────────────────────────── +// scoreConsecutiveHoursOfStudy +// ────────────────────────────────────────────────── + +func TestScoreConsecutiveHoursOfStudy(t *testing.T) { + slot := func(startMin, endMin int) models.ModuleSlot { + return models.ModuleSlot{StartMin: startMin, EndMin: endMin} + } + + tests := []struct { + name string + slots []models.ModuleSlot + maxHours int + want int + }{ + { + "empty slots", + nil, 4, 0, + }, + { + "single 2h class under 4h limit", + []models.ModuleSlot{slot(480, 600)}, 4, 0, + }, + { + "single 5h class (300 min), 1h over max=4", + []models.ModuleSlot{slot(480, 780)}, 4, + constants.ConsecutiveHoursPenaltyRate, + }, + { + "two back-to-back 2h classes = 4h, at limit", + // 09:00–11:00, 11:00–13:00 + []models.ModuleSlot{slot(540, 660), slot(660, 780)}, 4, 0, + }, + { + "two back-to-back 3h classes = 6h, 2h over max=4", + // 09:00–12:00, 12:00–15:00 + []models.ModuleSlot{slot(540, 720), slot(720, 900)}, 4, + 2 * constants.ConsecutiveHoursPenaltyRate, + }, + { + "two separate 3h classes with gap, each under limit", + // 09:00–12:00, 13:00–16:00 + []models.ModuleSlot{slot(540, 720), slot(780, 960)}, 4, 0, + }, + { + "two separate 3h classes with gap, each 1h over max=2", + // 09:00–12:00, 13:00–16:00 + []models.ModuleSlot{slot(540, 720), slot(780, 960)}, 2, + 2 * constants.ConsecutiveHoursPenaltyRate, + }, + { + "three consecutive 2h classes = 6h, 2h over max=4", + // 08:00–10:00, 10:00–12:00, 12:00–14:00 + []models.ModuleSlot{slot(480, 600), slot(600, 720), slot(720, 840)}, 4, + 2 * constants.ConsecutiveHoursPenaltyRate, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := scoreConsecutiveHoursOfStudy(tt.slots, tt.maxHours) + if got != tt.want { + t.Errorf("scoreConsecutiveHoursOfStudy() = %d, want %d", got, tt.want) + } + }) + } +} + +// ────────────────────────────────────────────────── +// scoreTimetableState +// ────────────────────────────────────────────────── + +func TestScoreTimetableState(t *testing.T) { + noRecordings := map[string]struct{}{} + + t.Run("all empty days contributes only TotalDistance", func(t *testing.T) { + state := models.TimetableState{ + Assignments: make(map[string]string), + TotalDistance: 50.0, + } + for d := range state.DaySlots { + state.DaySlots[d] = make([]models.ModuleSlot, 0) + } + req := lunchReq(720, 840, 4) + score := scoreTimetableState(state, noRecordings, req) + if score != 50.0 { + t.Errorf("got %v, want 50.0 (TotalDistance only)", score) + } + }) + + t.Run("single morning class earns lunch bonus", func(t *testing.T) { + // 08:00–09:00 on Monday, nothing in 12:00–14:00 lunch window + slot := models.ModuleSlot{StartMin: 480, EndMin: 540} + state := stateWithDay([]models.ModuleSlot{slot}) + req := lunchReq(720, 840, 4) + score := scoreTimetableState(state, noRecordings, req) + // calculateLunchGap = 120 >= 60 -> LunchBonus (-300), no gap, no consecutive penalty + wantFromDay := constants.LunchBonus + if score != float64(wantFromDay) { + t.Errorf("got %v, want %v (LunchBonus)", score, float64(wantFromDay)) + } + }) + + t.Run("class spanning lunch window earns lunch penalty", func(t *testing.T) { + // Class 12:00–14:00 covers entire lunch window + slot := models.ModuleSlot{StartMin: 720, EndMin: 840} + state := stateWithDay([]models.ModuleSlot{slot}) + req := lunchReq(720, 840, 4) + score := scoreTimetableState(state, noRecordings, req) + // calculateLunchGap = 0 < 60 -> NoLunchPenalty (+300) + // consecutive hours = 120 min < 240 min max -> 0 + wantFromDay := constants.NoLunchPenalty + if score != float64(wantFromDay) { + t.Errorf("got %v, want %v (NoLunchPenalty)", score, float64(wantFromDay)) + } + }) + + t.Run("large gap between classes adds gap penalty", func(t *testing.T) { + // 08:00–09:00, then 12:00–13:00 (gap = 180 min > 120 threshold) + s1 := models.ModuleSlot{StartMin: 480, EndMin: 540} + s2 := models.ModuleSlot{StartMin: 720, EndMin: 780} + state := stateWithDay([]models.ModuleSlot{s1, s2}) + req := lunchReq(720, 840, 4) + score := scoreTimetableState(state, noRecordings, req) + + // 60 min free after 13:00 (within lunch window) -> LunchBonus + // 180 min gap between classes > 120 threshold -> gap penalty = 100*(180-120)/60 = 100 + // Two separate 1h classes -> no consecutive penalty + expected := constants.LunchBonus + constants.GapPenaltyRate*float64(180-constants.GapPenaltyThreshold)/60 + if math.Abs(score-expected) > 0.01 { + t.Errorf("got %v, want ~%v (LunchBonus + gap penalty)", score, expected) + } + }) + + t.Run("excessive consecutive hours adds consecutive penalty", func(t *testing.T) { + // Three back-to-back 2h classes = 6h total (2h over max=4) + // 08:00–10:00, 10:00–12:00, 12:00–14:00 + s1 := models.ModuleSlot{StartMin: 480, EndMin: 600} + s2 := models.ModuleSlot{StartMin: 600, EndMin: 720} + s3 := models.ModuleSlot{StartMin: 720, EndMin: 840} + state := stateWithDay([]models.ModuleSlot{s1, s2, s3}) + req := lunchReq(720, 840, 4) + score := scoreTimetableState(state, noRecordings, req) + + // Classes run 08:00-14:00 with no breaks, so no lunch gap -> NoLunchPenalty (+300) + // No gaps between classes -> no gap penalty + // 6h consecutive = 2h over max (4h) -> +200 + expected := float64(constants.NoLunchPenalty) + float64(2*constants.ConsecutiveHoursPenaltyRate) + if math.Abs(score-expected) > 0.01 { + t.Errorf("got %v, want ~%v", score, expected) + } + }) + + t.Run("TotalDistance added to score", func(t *testing.T) { + // One morning slot (gets lunch bonus) with non-zero TotalDistance + slot := models.ModuleSlot{StartMin: 480, EndMin: 540} + state := stateWithDay([]models.ModuleSlot{slot}) + state.TotalDistance = 25.0 + req := lunchReq(720, 840, 4) + score := scoreTimetableState(state, noRecordings, req) + expected := float64(constants.LunchBonus) + 25.0 + if math.Abs(score-expected) > 0.01 { + t.Errorf("got %v, want %v (LunchBonus + TotalDistance)", score, expected) + } + }) +} diff --git a/website/api/optimiser/_test/api_test.go b/website/api/optimiser/_test/api_test.go index aeeb4e4069..b2fded0e26 100644 --- a/website/api/optimiser/_test/api_test.go +++ b/website/api/optimiser/_test/api_test.go @@ -98,7 +98,6 @@ func TestOptimiser_NoCollisionBetween2Lessons(t *testing.T) { t.Logf("✅ No Collision Between 2 Lessons Passed. Assignments: %v", result.Assignments) t.Logf(" Shareable link: %s", result.ShareableLink) - } func TestOptimiser_MultipleModulesWithFreeDays(t *testing.T) { @@ -305,14 +304,14 @@ func TestOptimiser_AllSlotsHaveAssignments(t *testing.T) { // helpers -// Day name constants for mapping +// Day name constants for mapping. var dayNames = []string{"Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"} // validate checks that the timetable satisfies all constraints: // - No time collisions between lessons on the same day // - All lessons are within earliestTime and latestTime bounds // - Free days have no lessons scheduled -// - Lessons marked as recordings should not appear in physical timetable +// - Lessons marked as recordings should not appear in physical timetable. func validateTimetable(t *testing.T, result models.SolveResponse, req models.OptimiserRequest) { t.Helper() diff --git a/website/package.json b/website/package.json index bd1060af26..b4ec91e515 100644 --- a/website/package.json +++ b/website/package.json @@ -17,6 +17,8 @@ "lint": "run-p \"lint:**\"", "lint:code": "oxlint -c oxlint.config.mjs api scripts src webpack", "lint:styles": "stylelint \"src/**/*.scss\" --syntax scss", + "lint:optimiser:code": "cd api/optimiser && golangci-lint run . ./_*/", + "lint:optimiser:styles": "cd api/optimiser && golangci-lint fmt", "test": "vitest run --coverage", "test:integration": "vitest run --config vitest.integration.config.ts", "test:watch": "vitest",