NotificationsYou must be signed in to change notification settings
Fork1.1k
Star11.6k

chore: add unit test for X11 eviction#18565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

spikecurtis merged 1 commit intomainfromspike/18263-eviction-unit-test

Jun 27, 2025

Merged

chore: add unit test for X11 eviction#18565

spikecurtis merged 1 commit intomainfromspike/18263-eviction-unit-test

Jun 27, 2025

Conversation

Copy link

Contributor

spikecurtis commentedJun 25, 2025•
edited
Loading

relates to#18263

Adds a unit test for X11 listener eviction when all ports in the allowed range are in use.

This was referencedJun 25, 2025

fix: cap max X11 forwarding ports and evict old#18561

Merged

fix: use memmap file system for TestServer_X11#18562

Merged

chore: move InProcNet to testutil#18563

Merged

Copy link

ContributorAuthor

spikecurtis commentedJun 25, 2025•
edited
Loading

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

This was referencedJun 25, 2025

chore: refactor TestServer_X11 to use inproc networking#18564

Merged

chore: test eviction with used ports#18566

Merged

chore: update X11 forward session usage when there is a connection#18567

Merged

github-actionsbot assignedspikecurtis

Jun 25, 2025

spikecurtis requested a review frommafredri

June 25, 2025 10:30

spikecurtis marked this pull request as ready for review

June 25, 2025 10:30

spikecurtis force-pushed thespike/18263-x11-network branch fromb60589d to212e2a5Compare

June 25, 2025 10:37

spikecurtis force-pushed thespike/18263-eviction-unit-test branch from7f2e241 toa68240dCompare

June 25, 2025 10:37

spikecurtis changed the title~~chore: unit test for X11 eviction~~chore: add unit test for X11 eviction

Jun 26, 2025

spikecurtis force-pushed thespike/18263-x11-network branch from212e2a5 toa272b09Compare

June 26, 2025 10:41

spikecurtis force-pushed thespike/18263-eviction-unit-test branch froma68240d to0d55551Compare

June 26, 2025 10:41

mafredri approved these changes

Jun 27, 2025

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nice test!

agent/agentssh/x11.go

		x.logger.Warn(sshCtx,"failed to get server connection")
		return-1,false
		}
		ctx:=slog.With(sshCtx,slog.F("session_id",fmt.Sprintf("%x",serverConn.SessionID())))

Copy link

Member

mafredriJun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is an under-appreciated feature of slog, nice to see it in action!

agent/agentssh/x11_test.go OutdatedShow resolvedHide resolved

agent/agentssh/x11_test.go

		// Calculate how many simultaneous X11 sessions we can create given the
		// configured port range.
		startPort:=agentssh.X11StartPort+agentssh.X11DefaultDisplayOffset
		maxSessions:=agentssh.X11MaxPort-startPort+1

Copy link

Member

mafredriJun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Suggestion: If we madeagentssh.X11MaxPort an option rather than constant, we could decrease the number sessions we have to create. This is definitely an optional suggestion, though.