Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,79 @@ make test-e2e
BASECAMP_BIN=./bin/basecamp bats e2e/
```

### Running basecamp connect against a local Basecamp

The connector's end-to-end scenarios in `e2e/connect` run the real binary
against the fake Basecamp in `internal/connector/fakebasecamp` as part of
`make test`. The same scenarios also run against a local Basecamp (BC3 in
development). Passing on both is the check that the fake behaves as Basecamp
does.

Prerequisites, all against the local Basecamp:

1. The event feed is enabled for the account, and Action Cable is served, so
`basecamp connect` can stream.
2. An operator profile, logged in as you:
`basecamp profile create rob --base-url http://3.basecamp.localhost:3001`.
3. An untrusted profile, logged in as another person on the project. The
agent takes no instructions from this person.
4. An agent connected under a profile of its own and set up in operator trust
(the default) to serve the project:

```bash
basecamp auth agent connect -P agent
basecamp connect setup -P agent --operator-profile rob --serve <project-id>
```

5. No connector running for that agent. The scenarios stop with an error if
`basecamp connect status -P agent` shows one, because that connector would
get the scenarios' mentions too.

Then run:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: This target only runs on Linux and macOS: the e2e/connect package is build-tagged for linux || darwin, so Windows contributors following these instructions will not run the scenarios. State the platform prerequisite here.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At CONTRIBUTING.md, line 134:

<comment>This target only runs on Linux and macOS: the `e2e/connect` package is build-tagged for `linux || darwin`, so Windows contributors following these instructions will not run the scenarios. State the platform prerequisite here.</comment>

<file context>
@@ -103,6 +103,79 @@ make test-e2e
+   `basecamp connect status -P agent` shows one, because that connector would
+   get the scenarios' mentions too.
+
+Then run:
+
+```bash
</file context>
Suggested change
Then run:
On Linux or macOS, run:


```bash
BASECAMP_BASE_URL=http://3.basecamp.localhost:3001 \
BASECAMP_CONNECT_DEV_AGENT_PROFILE=agent \
BASECAMP_CONNECT_DEV_OPERATOR_PROFILE=rob \
BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE=jason \
BASECAMP_CONNECT_DEV_PROJECT_ID=<project-id> \
make test-connect-dev
```

The target fails, and does not skip, when a variable is missing. It also
refuses a `BASECAMP_BASE_URL` that is not `localhost`, a loopback address or a
`*.localhost` host, because the scenarios post as your profiles.

What it does:

- It uses your config and your credentials (keyring included) as they are.
It never runs `auth agent connect` or `connect setup`, and it does not
change `connect.json`. It reads `connect.json` with `basecamp connect show`.
- Each scenario gives its connector a temporary `XDG_STATE_HOME`. The
connector's ledger and its per-agent instance lock are there. They are not
yours, so the scenario cannot read or move your connector's position, and
it cannot hold your lock.
- Each scenario posts a new message in the project as the operator, with
nobody subscribed. It comments on that message with `basecamp comments
create`, and mentions the agent as `[@agent](person:<id>)`. When the
scenario ends, it trashes the message and its comments.
- The scenarios run one at a time.

What it covers: a trusted mention handed off in one request line (field by
field, from the comment Basecamp returned), an untrusted mention discarded as
`untrusted_performer`, stdout that holds only NDJSON, a clean SIGINT, a
second connector for the same agent refused at once, and a request admitted
under `--hold` that is handed off after a `kill -9`, `connect release` and a
restart.

What stays fake-only: the scenarios that need a fault or a world only the
fake can make. These are a dropped or silent live connection, the poll lane
and the live lane published apart, a rate-limited token endpoint, failed or
held recording reads, an agent disconnected or connected on another computer,
tokens that last a second, a second agent, and `connect setup` itself. Each
one calls `fakeOnly` with its reason. The run against the local Basecamp
shows them as skipped, with that reason.

## Questions?

Open an issue for questions about contributing.
20 changes: 20 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,25 @@ test: check-toolchain
test-e2e: build
./e2e/run.sh

# Run the connector's end-to-end scenarios (e2e/connect) against a local
# Basecamp instead of the fake: the conformance check between the two.
# Scenarios that need a fault only the fake can produce are skipped, by name.
# See CONTRIBUTING.md, "Running basecamp connect against a local Basecamp".
# Required env vars:
# BASECAMP_BASE_URL - the local Basecamp (e.g. http://3.basecamp.localhost:3001)
# BASECAMP_CONNECT_DEV_AGENT_PROFILE - an agent connected and set up to serve the project
# BASECAMP_CONNECT_DEV_OPERATOR_PROFILE - the agent's operator, logged in
# BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE - someone else on the project, logged in
# BASECAMP_CONNECT_DEV_PROJECT_ID - the served project the scenarios post in
.PHONY: test-connect-dev
test-connect-dev: check-toolchain
@test -n "$(BASECAMP_BASE_URL)" || { echo "Set BASECAMP_BASE_URL (e.g. http://3.basecamp.localhost:3001); see CONTRIBUTING.md"; exit 1; }
@test -n "$(BASECAMP_CONNECT_DEV_AGENT_PROFILE)" || { echo "Set BASECAMP_CONNECT_DEV_AGENT_PROFILE (an agent connected and set up to serve the project); see CONTRIBUTING.md"; exit 1; }
@test -n "$(BASECAMP_CONNECT_DEV_OPERATOR_PROFILE)" || { echo "Set BASECAMP_CONNECT_DEV_OPERATOR_PROFILE (the agent's operator, logged in); see CONTRIBUTING.md"; exit 1; }
@test -n "$(BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE)" || { echo "Set BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE (someone else on the project, logged in); see CONTRIBUTING.md"; exit 1; }
@test -n "$(BASECAMP_CONNECT_DEV_PROJECT_ID)" || { echo "Set BASECAMP_CONNECT_DEV_PROJECT_ID (the served project the scenarios post in); see CONTRIBUTING.md"; exit 1; }
BASECAMP_CONNECT_DEV=1 $(GOTEST) -count=1 -failfast -v ./e2e/connect

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3: test-connect-dev omits $(BUILD_TAGS) that every other test recipe passes (test, race-test, test-coverage, etc. all use $(GOTEST) $(BUILD_TAGS)). Since make test runs go test -tags dev -v ./... and therefore includes ./e2e/connect, the fake-based runs are built with devBuild=true, which makes e2e/connect's binary() build the basecamp binary with -tags dev (dev_tag_test.go, main_test.go). This target builds the test binary without the tag, so against the local Basecamp the connector under test is a release-stub build — the conformance comparison is between two differently-tagged binaries, and any future dev-gated behavior silently diverges. Add $(BUILD_TAGS) to the recipe.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At Makefile, line 99:

<comment>`test-connect-dev` omits `$(BUILD_TAGS)` that every other test recipe passes (`test`, `race-test`, `test-coverage`, etc. all use `$(GOTEST) $(BUILD_TAGS)`). Since `make test` runs `go test -tags dev -v ./...` and therefore includes `./e2e/connect`, the fake-based runs are built with `devBuild=true`, which makes e2e/connect's `binary()` build the basecamp binary with `-tags dev` (dev_tag_test.go, main_test.go). This target builds the test binary without the tag, so against the local Basecamp the connector under test is a release-stub build — the conformance comparison is between two differently-tagged binaries, and any future dev-gated behavior silently diverges. Add `$(BUILD_TAGS)` to the recipe.</comment>

<file context>
@@ -79,6 +79,25 @@ test: check-toolchain
+	@test -n "$(BASECAMP_CONNECT_DEV_OPERATOR_PROFILE)" || { echo "Set BASECAMP_CONNECT_DEV_OPERATOR_PROFILE (the agent's operator, logged in); see CONTRIBUTING.md"; exit 1; }
+	@test -n "$(BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE)" || { echo "Set BASECAMP_CONNECT_DEV_UNTRUSTED_PROFILE (someone else on the project, logged in); see CONTRIBUTING.md"; exit 1; }
+	@test -n "$(BASECAMP_CONNECT_DEV_PROJECT_ID)" || { echo "Set BASECAMP_CONNECT_DEV_PROJECT_ID (the served project the scenarios post in); see CONTRIBUTING.md"; exit 1; }
+	BASECAMP_CONNECT_DEV=1 $(GOTEST) -count=1 -failfast -v ./e2e/connect
+
 # Record cassettes for happy-path replay tests.
</file context>
Suggested change
BASECAMP_CONNECT_DEV=1 $(GOTEST) -count=1 -failfast -v ./e2e/connect
BASECAMP_CONNECT_DEV=1 $(GOTEST) $(BUILD_TAGS) -count=1 -failfast -v ./e2e/connect


# Record cassettes for happy-path replay tests.
# Required env vars:
# BASECAMP_RECORD_TOKEN - valid API token for the target server
Expand Down Expand Up @@ -625,6 +644,7 @@ help:
@echo " race-test Run tests with race detector"
@echo " test-coverage Run tests with coverage report"
@echo " coverage Run tests with coverage and open in browser"
@echo " test-connect-dev Run the connector's e2e scenarios against a local Basecamp (see CONTRIBUTING.md)"
@echo " record-cassettes Record happy-path cassettes (TOKEN+TARGET+ACCOUNT+PROJECT)"
@echo " smoke Run pre-release smoke suite (BASECAMP_TOKEN=...)"
@echo " check-race-shards Test the race shards' union check"
Expand Down
160 changes: 85 additions & 75 deletions e2e/connect/connect_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,17 +64,15 @@ func askAgent() string {
// event published live reaches it.
func streaming(t *testing.T) (*Harness, *Connector) {
t.Helper()
h := NewHarness(t, world())
h.Setup(t)
c := h.Connect(t)
c.WaitStreaming(t, 1)
return h, c
f := newFakeTarget(t)
return f.h, streamingOn(t, f)
}

// The setup a person runs, through the CLI: the connection ceremony, then
// connect setup, which writes connect.json owner-only.
func TestSetupThroughTheCLIWritesConnectJSON(t *testing.T) {
t.Parallel()
fakeOnly(t, "it runs setup, which a local Basecamp's developer has run already, and connect.json there is theirs")
h := NewHarness(t, world())
h.Setup(t)

Expand Down Expand Up @@ -114,48 +112,55 @@ func TestSetupThroughTheCLIWritesConnectJSON(t *testing.T) {
// mention taken out of the words.
func TestATrustedMentionIsHandedOffInOneRequestLine(t *testing.T) {
t.Parallel()
h, c := streaming(t)
ev := comment(h, fakebasecamp.OperatorID, askAgent(), fakebasecamp.Live)
c.WaitFor(t, "the request line", func(o Output) bool { return len(o.RequestsFor(ev.ID)) > 0 })
out := c.Stop(t)
onEachTarget(t, func(t *testing.T, tg Target) {
c := streamingOn(t, tg)
p := tg.OperatorMentionsAgent(t)
c.WaitFor(t, "the request line", func(o Output) bool { return len(o.RequestsOn(p)) > 0 })
out := c.Stop(t)

pointers := out.PointersFor(ev.ID)
require.Len(t, pointers, 1)
assert.Equal(t, "live", pointers[0].Lane)
assert.Equal(t, ev.RecordingID, pointers[0].RecordingID)

events := out.EventsFor(ev.ID)
require.Len(t, events, 1)
assert.Equal(t, "admitted", events[0].State)
assert.Equal(t, "mentioned", events[0].Trigger)

requests := out.RequestsFor(ev.ID)
require.Len(t, requests, 1)
req := requests[0]
assert.Equal(t, "request", req.Type)
assert.Equal(t, "comment.created", req.EventType)
assert.Equal(t, "mentioned", req.Trigger)
assert.Equal(t, RequestRecording{
BucketID: fakebasecamp.ProjectID,
ProjectName: fakebasecamp.ProjectName,
RecordingID: ev.RecordingID,
Type: "Comment",
URL: h.Fake.URL() + "/" + strconv.FormatInt(fakebasecamp.AccountID, 10) + "/comments/" + strconv.FormatInt(ev.RecordingID, 10),
}, req.Recording)
assert.Equal(t, RequestReplyTo{Kind: "comment", RecordingID: kickoffID}, req.ReplyTo)
assert.Equal(t, fakebasecamp.OperatorID, req.RequesterID)
assert.Equal(t, fakebasecamp.OperatorName, req.RequesterName)
assert.True(t, req.Acknowledge, "a person asked for something")
// The mention leaves a space where it was, so no markup can close over
// the gap, and nothing else of the words changes.
assert.Equal(t, strings.Replace(askAgent(), fakebasecamp.Mention(fakebasecamp.AgentID), " ", 1), req.Content)
pointers := out.PointersOn(p)
require.Len(t, pointers, 1)
assert.Equal(t, "live", pointers[0].Lane, "Basecamp publishes live first")
assert.Equal(t, p.BucketID, pointers[0].BucketID)
assert.Equal(t, p.AuthorID, pointers[0].CreatorID)

events := out.EventsOn(p)
require.Len(t, events, 1)
assert.Equal(t, pointers[0].EventID, events[0].EventID)
assert.Equal(t, "admitted", events[0].State)
assert.Equal(t, "mentioned", events[0].Trigger)
assert.Equal(t, p.URL, events[0].RecordingURL)

requests := out.RequestsOn(p)
require.Len(t, requests, 1)
req := requests[0]
assert.Equal(t, pointers[0].EventID, req.EventID)
assert.Equal(t, "request", req.Type)
assert.Equal(t, "mentioned", req.Trigger)
assert.Equal(t, RequestRecording{
BucketID: p.BucketID,
ProjectName: p.ProjectName,
RecordingID: p.RecordingID,
Type: "Comment",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: Compare p.Type here instead of hardcoding "Comment"; otherwise the dev conformance test cannot detect a mismatch between Basecamp’s returned type and the connector’s request line.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At e2e/connect/connect_test.go, line 144:

<comment>Compare `p.Type` here instead of hardcoding `"Comment"`; otherwise the dev conformance test cannot detect a mismatch between Basecamp’s returned type and the connector’s request line.</comment>

<file context>
@@ -114,48 +112,55 @@ func TestSetupThroughTheCLIWritesConnectJSON(t *testing.T) {
+			BucketID:    p.BucketID,
+			ProjectName: p.ProjectName,
+			RecordingID: p.RecordingID,
+			Type:        "Comment",
+			Title:       p.Title,
+			URL:         p.URL,
</file context>
Suggested change
Type: "Comment",
Type: p.Type,

Title: p.Title,
URL: p.URL,
}, req.Recording)
assert.Equal(t, RequestReplyTo{Kind: "comment", RecordingID: p.ParentID}, req.ReplyTo)
assert.Equal(t, p.AuthorID, req.RequesterID)
assert.Equal(t, p.AuthorName, req.RequesterName)
assert.True(t, req.Acknowledge, "a person asked for something")
// The mention leaves a space where it was, so no markup can close over
// the gap, and nothing else of the words changes.
assert.Equal(t, p.Instruction, req.Content)
})
}

// Basecamp publishes an event on both lanes: live first, and on the poll
// lane the connector walks when it reconnects. It is one event, so it is
// one pointer line and one request line.
func TestAnEventOnBothLanesIsHandedOffOnce(t *testing.T) {
t.Parallel()
fakeOnly(t, "it decides what each lane serves and drops the live connection")
h, c := streaming(t)
ev := comment(h, fakebasecamp.OperatorID, askAgent(), fakebasecamp.Both)
c.WaitFor(t, "the request line", func(o Output) bool { return len(o.RequestsFor(ev.ID)) > 0 })
Expand Down Expand Up @@ -188,51 +193,56 @@ func TestAnEventOnBothLanesIsHandedOffOnce(t *testing.T) {
// connector says so in an event line, and hands nothing off.
func TestAnUntrustedMentionIsDiscardedAndNotHandedOff(t *testing.T) {
t.Parallel()
h, c := streaming(t)
ev := comment(h, BystanderID, askAgent(), fakebasecamp.Live)
c.WaitFor(t, "the event line", func(o Output) bool { return len(o.EventsFor(ev.ID)) > 0 })
out := c.Stop(t)
onEachTarget(t, func(t *testing.T, tg Target) {
c := streamingOn(t, tg)
p := tg.UntrustedMentionsAgent(t)
c.WaitFor(t, "the event line", func(o Output) bool { return len(o.EventsOn(p)) > 0 })
out := c.Stop(t)

events := out.EventsFor(ev.ID)
require.Len(t, events, 1)
assert.Equal(t, "discarded", events[0].State)
assert.Equal(t, "untrusted_performer", events[0].Reason)
assert.Equal(t, BystanderID, events[0].RequesterID)
assert.Empty(t, out.RequestsFor(ev.ID))
assert.Empty(t, out.Requests())
events := out.EventsOn(p)
require.Len(t, events, 1)
assert.Equal(t, "discarded", events[0].State)
assert.Equal(t, "untrusted_performer", events[0].Reason)
assert.Equal(t, p.AuthorID, events[0].RequesterID)
assert.Empty(t, out.RequestsOn(p))
assert.Empty(t, out.RequestsAbout(p), "nothing on the recording is handed off")
})
}

// Stdout is a protocol a program parses: every line one JSON object of a
// shape the protocol has, and nothing else. What a person reads goes to
// stderr, as slog lines, starting with the connector saying it runs.
func TestStdoutIsOnlyNDJSONAndStderrSaysItRuns(t *testing.T) {
t.Parallel()
h, c := streaming(t)
trusted := comment(h, fakebasecamp.OperatorID, askAgent(), fakebasecamp.Live)
untrusted := comment(h, BystanderID, askAgent(), fakebasecamp.Live)
c.WaitFor(t, "both verdicts and the request", func(o Output) bool {
return len(o.RequestsFor(trusted.ID)) > 0 && len(o.EventsFor(untrusted.ID)) > 0
})
out := c.Stop(t)
onEachTarget(t, func(t *testing.T, tg Target) {
c := streamingOn(t, tg)
trusted := tg.OperatorMentionsAgent(t)
untrusted := tg.UntrustedMentionsAgent(t)
c.WaitFor(t, "both verdicts and the request", func(o Output) bool {
return len(o.RequestsOn(trusted)) > 0 && len(o.EventsOn(untrusted)) > 0
})
out := c.Stop(t)

require.NotEmpty(t, out.Lines)
for _, l := range out.Lines {
assert.True(t, json.Valid([]byte(l.Raw)), "not JSON: %q", l.Raw)
assert.NoError(t, l.Err, "%q", l.Raw)
}
assert.NotEmpty(t, out.Pointers())
assert.NotEmpty(t, out.Events())
assert.NotEmpty(t, out.Requests())

running := out.Logged(MsgRunning)
require.Len(t, running, 1)
assert.Equal(t, "INFO", running[0].Level)
assert.Equal(t, Agent, running[0].Attrs["profile"])
assert.Equal(t, strconv.FormatInt(fakebasecamp.AccountID, 10), running[0].Attrs["account"])
assert.Equal(t, strconv.FormatInt(fakebasecamp.AgentID, 10), running[0].Attrs["agent_person_id"])
for _, l := range out.Logs {
assert.NotEmpty(t, l.Msg, "stderr line is not a slog record: %q", l.Raw)
}
require.NotEmpty(t, out.Lines)
for _, l := range out.Lines {
assert.True(t, json.Valid([]byte(l.Raw)), "not JSON: %q", l.Raw)
assert.NoError(t, l.Err, "%q", l.Raw)
}
assert.NotEmpty(t, out.Pointers())
assert.NotEmpty(t, out.Events())
assert.NotEmpty(t, out.Requests())

agent := tg.Agent()
running := out.Logged(MsgRunning)
require.Len(t, running, 1)
assert.Equal(t, "INFO", running[0].Level)
assert.Equal(t, agent.Profile, running[0].Attrs["profile"])
assert.Equal(t, strconv.FormatInt(agent.AccountID, 10), running[0].Attrs["account"])
assert.Equal(t, strconv.FormatInt(agent.PersonID, 10), running[0].Attrs["agent_person_id"])
for _, l := range out.Logs {
assert.NotEmpty(t, l.Msg, "stderr line is not a slog record: %q", l.Raw)
}
})
}

// cursorBefore reports whether a poll position the fake issued is before
Expand Down
2 changes: 1 addition & 1 deletion e2e/connect/connector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func (h *Harness) Connect(t *testing.T, extra ...string) *Connector {
// flags, in the harness's home.
func (h *Harness) ConnectProfile(t *testing.T, p Profile, extra ...string) *Connector {
t.Helper()
return h.Start(t, h.Home, append([]string{"connect", "-P", p.Name}, extra...)...)
return h.Start(t, h.Dir, append([]string{"connect", "-P", p.Name}, extra...)...)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P3: The doc comments directly above Connect and ConnectProfile still say the command runs "in the harness's home", but this line now starts it in h.Dir. For the dev target Home is empty and Dir is the temporary working directory, so the comments are wrong there. Update both to say the commands run in the harness's working directory.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At e2e/connect/connector_test.go, line 75:

<comment>The doc comments directly above `Connect` and `ConnectProfile` still say the command runs "in the harness's home", but this line now starts it in `h.Dir`. For the dev target `Home` is empty and `Dir` is the temporary working directory, so the comments are wrong there. Update both to say the commands run in the harness's working directory.</comment>

<file context>
@@ -72,7 +72,7 @@ func (h *Harness) Connect(t *testing.T, extra ...string) *Connector {
 func (h *Harness) ConnectProfile(t *testing.T, p Profile, extra ...string) *Connector {
 	t.Helper()
-	return h.Start(t, h.Home, append([]string{"connect", "-P", p.Name}, extra...)...)
+	return h.Start(t, h.Dir, append([]string{"connect", "-P", p.Name}, extra...)...)
 }
 
</file context>

}

// Start starts a CLI command in the background, in dir, with the harness's
Expand Down
Loading
Loading