feat: init feature set and testsuite#197
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces a comprehensive feature set specification and test suite system to document and validate server behavior requirements. The system uses RFC 2119 requirement levels (MUST/SHOULD/MAY) with executable Go test cases that validate implementations against specifications.
Changes:
- Added feature set framework with 4 initial feature sets: FS-0000-INIT, FS-0001-AUTH, FS-0002-USER, and FS-0003-SESSION
- Implemented test suite infrastructure with dependency management, topological sorting, and Mermaid visualization
- Updated proto definitions for session management (renamed RevokeUserSession, added GetDevice, enhanced ListUserSessions)
- Modified server information fields in base.proto (breaking changes to URL field names)
- Enhanced documentation with interactive frontmatter rendering and test suite guides
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| proto/librarian/sephirah/v1/tiphereth.proto | Added session filtering, device management endpoints; renamed DeleteUserSession to RevokeUserSession; modified UserSession.device_id to optional |
| proto/librarian/sephirah/v1/sephirah_service.proto | Updated authentication mode comments from "Normal" to "access_token"/"anonymous" for clarity |
| proto/librarian/sephirah/v1/base.proto | Breaking changes: renamed logo_url/background_url to logo_image_url/background_image_url; added is_registerable field |
| go.mod, go.sum | Added dependencies for kratos framework, grpc utilities, and test infrastructure |
| docs/testsuite.md | New documentation explaining test suite usage, execution, and troubleshooting |
| docs/index.html | Added frontmatter plugin and rendering logic for feature set metadata tables |
| docs/feature_sets/*.go | Test implementations for 4 feature sets with 40+ test cases |
| docs/feature_sets/*.md | Markdown specifications defining requirements with RFC 2119 keywords |
| docs/feature_sets/AGENTS.md | Comprehensive guide for writing and maintaining feature sets |
| cmd/testsuite/main.go | CLI tool supporting 'run' and 'tree' subcommands for test execution and visualization |
| Makefile | Added testsuite-tree target (with naming issue) to generate dependency graphs |
| docs/README.md, docs/_sidebar.md | Updated navigation to include feature sets and test suite documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
docs/feature_sets/FS-0002-USER.go
Outdated
| state := getUserState(g) | ||
|
|
||
| // Generate random username and password | ||
| rand.Seed(time.Now().UnixNano()) |
There was a problem hiding this comment.
Using rand.Seed() and rand.Int63() from math/rand is deprecated in Go 1.20+. Since Go 1.20, the global random number generator is automatically seeded. The code should either use rand.New() with a source, or simply remove the rand.Seed() call and use the global generator directly. For Go 1.20+, you can use rand.Int63() without seeding.
| rand.Seed(time.Now().UnixNano()) |
| */ | ||
| service LibrarianSephirahService { | ||
| // Allow anonymous call, use accessToken to get complete information | ||
| // `anonymous` `access_token` Allow anonymous call, use accessToken to get complete information |
There was a problem hiding this comment.
The comment format anonymous access_token is inconsistent with standard protobuf comment style. Consider using a more conventional format such as a tag-based system (e.g., @auth: anonymous, access_token) or a structured comment format that clearly delineates the authentication modes.
|
|
||
| message RefreshTokenRequest { | ||
| // Always ignore this if client don't impl device info feature. | ||
| // Ignore this if client don't impl device info feature. |
There was a problem hiding this comment.
Grammar issue: "don't impl" should be "don't implement" or "doesn't implement". The abbreviated form "impl" is informal for documentation.
docs/feature_sets/FS_overview.md
Outdated
|
|
||
| Feature Sets describe collections of related features in this system. | ||
|
|
||
| ## writing guide |
There was a problem hiding this comment.
The header "writing guide" should use title case: "Writing Guide" for consistency with other headers in documentation files.
| ## writing guide | |
| ## Writing Guide |
| if err != nil { | ||
| t.Fatalf("sortTestCases() error = %v", err) | ||
| } | ||
|
|
||
| if testCases[0].ID != "FS-0001-TEST-A" { | ||
| t.Errorf("TestCases[0].ID = %v, want FS-0001-TEST-A (missing dependency ignored)", testCases[0].ID) | ||
| } | ||
|
|
||
| if testCases[1].ID != "FS-0002-TEST-B" { | ||
| t.Errorf("TestCases[1].ID = %v, want FS-0002-TEST-B", testCases[1].ID) | ||
| } |
There was a problem hiding this comment.
The TestSortTestCasesWithMissingDependency test expects that missing dependencies should be silently ignored (test passes without error), but the actual implementation in sortTestCases() returns an error when a dependency is not found. This is a discrepancy between the test expectation and the implementation. Either the test should expect an error, or the implementation should ignore missing dependencies.
| if err != nil { | |
| t.Fatalf("sortTestCases() error = %v", err) | |
| } | |
| if testCases[0].ID != "FS-0001-TEST-A" { | |
| t.Errorf("TestCases[0].ID = %v, want FS-0001-TEST-A (missing dependency ignored)", testCases[0].ID) | |
| } | |
| if testCases[1].ID != "FS-0002-TEST-B" { | |
| t.Errorf("TestCases[1].ID = %v, want FS-0002-TEST-B", testCases[1].ID) | |
| } | |
| if err == nil { | |
| t.Fatalf("expected sortTestCases() to return an error due to missing dependency, but got nil") | |
| } |
Makefile
Outdated
| dart pub global activate protoc_plugin 22.5.0 | ||
|
|
||
| generate: clean buf go rust dart | ||
| generate: clean buf go rust dart dependency-tree |
There was a problem hiding this comment.
The Makefile references a target dependency-tree on line 14, but the actual target is named testsuite-tree on line 27. This will cause the make generate command to fail.
| generate: clean buf go rust dart dependency-tree | |
| generate: clean buf go rust dart testsuite-tree |
| string password = 2; | ||
| // Always ignore this if client don't impl device info feature. | ||
| // Otherwise, re-login after registered device with this and every time after | ||
| // Ignore this if client don't impl device info feature. |
There was a problem hiding this comment.
Grammar issue: "don't impl" should be "don't implement" or "doesn't implement". The abbreviated form "impl" is informal for documentation.
| optional string logo_image_url = 4; | ||
| optional string background_image_url = 5; |
There was a problem hiding this comment.
The field names have been changed from logo_url and background_url to logo_image_url and background_image_url. This is a breaking change that requires careful consideration. Ensure that all consumers of this API are updated to handle the new field names. Consider using field deprecation instead if backward compatibility is important.
docs/feature_sets/FS_overview.md
Outdated
|
|
||
| ## writing guide | ||
|
|
||
| - FSD contains a markdown file and a gherkin file with the same filename |
There was a problem hiding this comment.
Inconsistency: The documentation mentions "FSD contains a markdown file and a gherkin file" but the actual implementation only uses markdown files (.md) and Go files (.go). There are no Gherkin files (.feature) in the feature_sets directory. This documentation should be updated to reflect the actual implementation.
| - FSD contains a markdown file and a gherkin file with the same filename | |
| - Each FSD consists of a markdown specification file (.md) and corresponding Go implementation file(s) (.go) |
No description provided.