test: Obliterate & revive integration tests (fixes #8071) #9266
Conversation
* main: lib/model: Use a single lock (phase two: cleanup) (syncthing#9276) build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279) lib/model: Use a single lock (syncthing#9275) cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281) cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166) lib/nat: Fix test build failure (ref syncthing#9010) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271) lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010) gui, man, authors: Update docs, translations, and contributors gui: Show folder/device status on small screens (syncthing#8643) lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
There was a problem hiding this comment.
Great, tests that pass and in reasonable time - few minor/optional comments, overall lgtm
I have some sympathy for the old scenarios of throwing pretty wild scenarios at it, I think it did provide some insights in the past. Also I have vague memories of the benchmarks being useful. Not arguing against anything, just some nice passing words for those :D
| addrExp := regexp.MustCompile(`GUI and API listening on ([^\s]+)`) | ||
| myIDExp := regexp.MustCompile(`My ID: ([^\s]+)`) | ||
| tcpAddrExp := regexp.MustCompile(`TCP listener \((.+)\) starting`) | ||
| go func() { |
There was a problem hiding this comment.
This goroutine never stops as far as I see. Not impactful, but less noise in goroutine stacks is always nice, so I think would be nice to limit it's lifetime to the command.
There was a problem hiding this comment.
It should exit when sc.Read fails, which will happen when the io.Reader returns an error (io.EOF or similar, when the process exits), so this should only live for the lifetime of the process.
There was a problem hiding this comment.
I'd like it to be scoped to the lifetime of a tests though - I don't see that reader (from io.Pipe in startInstance) being closed at the end of a test. What am I missing?
There was a problem hiding this comment.
Refactoring to happen, take this all with a grain of salt for now.
lib/rc/rc.go
Outdated
There was a problem hiding this comment.
There's not much left in this package - it's now just an API helper and most of the process handling now lives in ./test. My gut feeling is to remove this package and also move the remaining code to test.
There was a problem hiding this comment.
Hi, longtime Go programmer and SyncThing user here.
I have been reading the source code and pull requests recently, and humbly offer some comments for your consideration. Please excuse my presumptuousness, and feel free to entirely ignore me if you don't find the comments helpful.
Cheers, and thanks for working on SyncThing.
| addrExp := regexp.MustCompile(`GUI and API listening on ([^\s]+)`) | ||
| myIDExp := regexp.MustCompile(`My ID: ([^\s]+)`) | ||
| tcpAddrExp := regexp.MustCompile(`TCP listener \((.+)\) starting`) | ||
| go func() { |
There was a problem hiding this comment.
It should exit when sc.Read fails, which will happen when the io.Reader returns an error (io.EOF or similar, when the process exits), so this should only live for the lifetime of the process.
| go func() { | ||
| for sc.Scan() { | ||
| line := sc.Text() | ||
| lr.log.WriteString(line + "\n") |
There was a problem hiding this comment.
nit:
could be lr.log.Write(sc.Bytes()) and lr.log.WriteByte('\n') to save allocations.
similarly below you could use the regexp FindSubmatch method instead of FindStringSubmatch and match on the line bytes, instead of a string
| }) | ||
|
|
||
| // Wait up to 30 seconds to get the device ID, which comes first. | ||
| select { |
There was a problem hiding this comment.
minor suggestion:
I'd be inclined to bundle all the concurrency related to this operation in the one function, so you have something like
func readSyncthingMetadata(r io.Reader) (*syncthingMetadata, error)
which handles all the timeouts etc. Then you don't need the struct with all the channels, the scanner goroutine can set the fields of a syncthingMetadata struct in place as it scans them, and then when it has populated all fields it can signal to the main goroutine that it's ready to be returned.
this would make error handling more natural as well, I think
| if m := myIDExp.FindStringSubmatch(line); len(m) == 2 { | ||
| id, err := protocol.DeviceIDFromString(m[1]) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
suggestion:
Add some context to these panic messages - the full line that has been matched would probably be good. Otherwise the parse error on its own might be bit mysterious.
I'd be inclined to actually return these rather than panicking, though. Since this goroutine will have a different stack to the one that is starting the instance, it might get painful to figure out which instance isn't starting properly.
test/sync_2dev_test.go
Outdated
| } | ||
| } | ||
|
|
||
| srcDone := make(chan struct{}) |
There was a problem hiding this comment.
suggest using sync.WaitGroup instead
test/sync_2dev_test.go
Outdated
| lastEventID := 0 | ||
| remoteCompletion := 0.0 | ||
| remoteIndexUpdated := false | ||
| loop: |
There was a problem hiding this comment.
If you're only using this label for break loop then you can lose the label and just return, since nothing happens after the loop.
* main: (24 commits) lib/ignore: Refactor out result type (syncthing#9343) build: Testing infra images for infra-* branches lib/versioner: Expand tildes in version directory (fixes syncthing#9241) (syncthing#9327) lib/scanner: Prevent sync-conflict for receive-only local modifications (syncthing#9323) gui, man, authors: Update docs, translations, and contributors Fix website security link in README.md (syncthing#9325) cmd/syncthing: Add CLI completion functionality (fixes syncthing#8616) (syncthing#9226) lib/api: Save session & CSRF tokens to database, add option to stay logged in (fixes syncthing#9151) (syncthing#9284) Update dependencies (syncthing#9321) gui: Always inform about loading data in Restore Versions modal (syncthing#9317) lib/build: Allow semver build in version regex (fixes syncthing#9267) (syncthing#9316) gui: Keep short deviceID length consistent + xrefs (fixes syncthing#9313) (syncthing#9314) build(deps): bump actions/download-artifact from 3 to 4 (syncthing#9294) build(deps): bump actions/upload-artifact from 3 to 4 (syncthing#9293) gui, man, authors: Update docs, translations, and contributors gui, lib/scanner: Improve scan progress indication (ref syncthing#8331) (syncthing#9308) lib/protocol: handle empty names in unixOwnershipEqual (fixes syncthing#9039) (syncthing#9306) gui, man, authors: Update docs, translations, and contributors etc/linux-desktop: use double dash for long options (syncthing#9301) lib/connections: Skip allocation in check for missing port (syncthing#9297) ...
Our integration tests have been broken and not run (or even compiled) for many years. This removes them and revives the concept with a simple two-device sync setup.
The reason the old tests fell into disuse is that the concept was misguided. We built complex, unmaintainable scenarios that were flaky and took a long time to run (and then failed for random reasons). We tried to have set configs on disk, so only one instance could run at a time, and those configs were sometimes changed and then changed back in tests...
The new tests I added are perhaps not enormously valuable, but demonstrate the concept of using ephemeral Syncthing instances that can run concurrently and don't interfere with each other.
All in all, I think this is an improvement, even though almost everything of the old is simply thrown away.
The (new) integration tests are now run as part of the build, on Linux, macOS and Windows.