feat(conform): add opt-in group mutation test by thirschel-microsoft · Pull Request #118 · xregistry/server

thirschel-microsoft · 2026-10-20T15:43:11Z

Summary

Add an explicitly authorized mutations conformance run that exercises one ordinary Group create/read/delete lifecycle.
Require --allow-mutations and exactly one target before any network request; existing default, all, smoke, and entities runs remain read-only.
Gate the lifecycle on verified capabilities and model support, using a unique run-owned Group ID and ownership label.
Protect cleanup by requiring a verified creation epoch, then re-checking ownership, emptiness, and the unchanged epoch before deletion. Unknown or residual data is reported rather than deleted.
Add focused lifecycle, cleanup-failure, CLI safety, text/JSON output, and help documentation coverage.

Testing

go test -count=1 ./cmds/xr ./cmds/xr/xrlib
go test -count=1 ./tests -run '^(TestXRConform.*|TestGroupCreateIgnoresEpochAndCollisionPreservesExisting)$'
Repeated both focused scopes with -race
make docs xrlint errors
XR_SPEC=/tmp/xregistry-spec-pinned make -B xrlint errors using spec snapshot ffac2f5ad77bd86db01fa8fca5866d0d3d8a761c
Verified generated files remained unchanged and all modified Go files are gofmt clean

duglin · 2026-10-20T16:01:56Z

@thirschel-microsoft can you rebase? I find it kind of disturbing that github shows lots of old code in your files when I expand them - it'll probably just merge them but the latest changes I merged are so big I'd prefer to double-check.

duglin · 2026-10-20T16:03:56Z

 	}
 }

+func conformMutationSelected(names []string) bool {


You can replace calls to this with calls to common.ArrayContains()

duglin · 2026-10-20T16:09:06Z

 	nTD = NewTD(td, "Verifying model MUST work")
-	nTD.NoError(model.Verify())
+	verifyErr := model.Verify()
+	nTD.NoError(verifyErr)


Let's just use NoErrorStop() instead and then we can remove the "if" around the reg.Model = model

duglin · 2026-10-20T16:23:53Z

+		td.Skip()
+		return
+	}
+	if !reg.Capabilities.FlagEnabled("epoch") {


Seems like we could still test delete even w/o the ?epoch flag... both w/o any epoch checking and using the http body instead of the query param

Yeah thats fair. I removed the ?epoch requirement.

duglin · 2026-10-20T16:24:38Z

+		td.Skip()
+		return
+	}
+	if reg.Model == nil {


Shouldn't the td.DependsOn(TestModel) prevent the need for this?

You're right. Removed this check

duglin · 2026-10-20T16:32:04Z

+	}))
+
+	preflightTD := NewTD(td, "Confirm mutation destination is absent")
+	preflightRes, _ := reg.HttpDoNoRedirect(VerboseCount > 2, "GET", path, nil)


Why did you want to not allow a redirect? It's a bit odd, but it's possible that someone is hosting an xReg thru some kind of proxy/shadow site and they blindly do a redirect based on the URLs, and so if the redirect fails point to a non-existing entity then that'll return a 404 after we get the 3xx, yielding the same net result, right?

My concern was something replaying the write. I haven't thought about all these different xreg setups but Im open to thoughts here

I think that's out of our control/concern - as long as we generate the proper number of "writes", we then need to react to the server just like any other client would. Which means, follow the redirect that the server told us to.

duglin · 2026-10-20T17:33:15Z

+   ├─ PASS: Create run-owned Group
+   ├─ PASS: Read run-owned Group
+   └─ FAIL: Clean up run-owned Group
+      ├─ FAIL: Cleanup GET MUST return 200 or 404


Can we show what it did return to help people debug it a little?

I think using td.Log() will help since those should only show on failure.

Yeah td.Log() makes sense. I added some td.Log() logs

duglin · 2026-10-20T17:38:41Z

+└─ PASS: TestTDGroupCreateReadDelete (skip:1)
+   ├─ PASS: TestModel
+   ├─ PASS: TestCapabilities (skip:1)
+   ├─ PASS: Confirm mutation destination is absent


perhaps instead of "Confirm mutation destination is absent", we could make it "Confirm target Group is absent" to be a bit more user friendly?

duglin · 2026-10-20T17:40:14Z

+					owner := ""
+					groupGets := 0
+					deletes := 0
+					server := httptest.NewServer(http.HandlerFunc(


I'm not 100% sure I'm following this but it looks like you're introducing a proxy to watch the http flows between td and the server so you can verify something - a few comments would help. But, I'm trying to understand why we need this when I would think the td stuff would need to do similar validation and then fail if the bits you're looking for are not correct, no? I assume the if groupGets == 2 is critical in what's going on, but it's not clear to me how it all fits in - I'm guessing it's related to testing the deletion of the Group not happening. And if that's all that's going on, I think comments would help the next person reading this.

This uses the real server but I made it so the second GET fails. That acts as the check before deleting the group so the test makes sure we leave it alone when we can’t verify ownership. Added a comment to explain that.

duglin · 2026-10-20T17:44:46Z

+	return "", nil
+}
+
+func conformMutationGroupState(body []byte, groupModel *xrlib.GroupModel,


With the commit I merged today I'm on the verge of allowing for all of the server code to be available to the client (w/o the mysql and http layers). A fair amount of it is actually there already due to me moving it into the "shared_xxx" files in common. One of the reasons I'm headed this direction is to allow us to call the verification funcs that the server defines but from the client, in order to avoid duplication of logic which is what I think the funcs below is starting to do. IOW, I'd like to see if we can get to the point where we can just have things like:

xErr := group.Validate()

in our client code for instances and:

xErr := model.Verify()

for model stuff. That should handle most of the checks. Some of them (e.g. COLLECTION* attrs) might need new code, but I think it would be good to have that logic live outside of the TD stuff in case we want to make it available to other apps that want to use our stuff as an SDK (another long-term goal of mine).

From a "registry" dir perspective I think the code is ready. But I need to tweak some client-side (xrlib) code first to remove duplication and other things first. I'm hoping to get that done over the next day or so. Then all of the common/shared_xxx files will be moved back into "registry" and the cli will then be able to import it as a shared package. "common" can then return to being just a package for utils.

duglin · 2026-10-20T17:55:11Z

I know it's super anal, but I think in the long run it's easier for new people when they join... but can you use "dirs" and "files" in the model instead of "widgets"? Unless there's a specific reason to deviate, I think eliminating one extra thing to think about (e.g. are widgets groups or resources? or to have a changing model names per new dev) eases the learning curve.

duglin · 2026-10-21T16:58:32Z

+	if isNew {
+		// A create request must silently ignore any supplied epoch value.
+		delete(obj, "epoch")
+	}


I think it would be better to solve this generically by modifying RemoveReadOnlyImmutable like this:

- continue + + // however, there is one very special case for "epoch". + // On create, delete it no matter what. In edge cases where + // it's not even a uint we don't want to see a "bad type" error. + // e.OriginObj being empty is a sign that it's new. + if key != "epoch" || len(e.OriginObject) != 0 { + continue + }

at line 774 (the if-statement that treats epoch as special). This will work for all entity types.

You can then add an XHTTP call to the end of TestHTTPEpoch() to do: PUT /?inline=* with a full tree (new dirs/files/versions) specifying "epoch": "foo" for all (except registry) and verify no error was generated and epoch is 1 for all except registry.

duglin · 2026-10-26T04:12:47Z

+			if key != "epoch" || e.OriginObject == nil ||
+				len(e.OriginObject) != 0 {
+				continue
+			}


I did some checking to see why this isn't set in all cases and I decided that only setting it when we edit the entity isn't good - it should be set all the time so the calling code doesn't need to think about it. I create a new PR for the fix: #122 and will merge it tomorrow once I double check it. I ran your new tests and they work but I'll leave those in your PR since you wrote them. I'll let you know when you can rebase.

merged. I'll try to complete the review of the PR later today

duglin

I've only had time to look at the code related to the "server testing" itself, not the "td logic testing" yet, but wanted to send what I have so far....

duglin · 2026-10-27T22:25:49Z

+			Error("--run mutations requires --allow-mutations")
+		}
+		if len(servers) != 1 {
+			Error("--run mutations requires exactly one target")


Any reason we can't run the mutations on each server?

Fair point. removed this restriction

duglin · 2026-10-27T22:35:25Z

+	}
+	groupType, groupModel := conformMutationGroupModel(reg.Model)
+	if groupModel == nil {
+		td.Skip("No Group Type supports run-owned creation - leaving")


I tried to run this against the server immediately after I built it and I got this skip msg. Took me a few seconds to realize why... I had no model defined at all. It would be nice if the output told the user what we're looking for to make this determination, or what was missing, so they know what to change to enable it. I think some td.Msg lines before the td.Skip here would be sufficient for the general requirements... or in conformMutationGroupModel if we can be more specific about what's missing.

I added some diagnostic output that should give better hints to the user about what they are missing in these cases

duglin · 2026-10-27T23:30:24Z

 	}
 }

+func TestTDGroupCreateReadDelete(td *TD) {


let's rename this to TestMutations to align with the --run flag.
While we're at it, rename TestTDAll to just TestAll.
Actually let's remove the "TD" from all user-facing tests (and keep TestTD for TD testing funcs). I never intended for the TestTD funcs to be visible to the user, they're for our own testing. But somehow some (like All) ended up switching from a td-test func into a user facing one.

duglin · 2026-10-27T23:37:56Z

 	if config.Output != "text" && config.Output != "json" {
 		Error("--output must be one of: text, json")
 	}
+	if common.ArrayContains(runNames, "mutations") {


I'm not sure how I feel about requiring --allow-mutations. I kind of understand the desire to avoid touching a registry if the user didn't really know what they're doing, but at the same time, between the "entities" capability and the test creating its own group instances, and then cleaning them up, it seems like it should be safe. I'll think about it more, however, in the meantime, I think we should make TestAll actually test everything, including "mutations" and "entities" - but we can make mutations conditional on --allow-mutations for the time being - and showing a skip if they didn't include the flag.

I changed TestAll to include entities and mutations

duglin · 2026-10-27T23:58:24Z

 	nTD = NewTD(td, "Verifying model MUST work")
-	nTD.NoError(model.Verify())
+	nTD.NoErrorStop(model.Verify())
+	reg.Model = model


let's remove this setting of the model here. My current thinking is that the tests shouldn't really be setting/saving values in the Entities in most cases - that should be done by the common Registry entity processing logic. Instead, if a test needs the model (but isn't explicitly testing the model) then it should call reg.GetModel() and my recent changes should automatically load it if it's not already available. This does a few things:

it doesn't require TestModel to be run before a test uses the model. Even if we end up always making TestModel a dependency, I think it's a better design to try to have a clean separation between "stuff we're testing" and "stuff we need to move around/reason about the target registry".

GetModel(), while not yet, will eventually retrieve the model via GET /model or GET /?inline=model as needed. Meaning it'll try all possible mechanisms to get the model, while (as of now) TestModel only tries GET /model. Yes I do want TestModel to test both paths too eventually, but if the tests fails for some reason we shouldn't be dependent on it saving the model for the rest of the tests. Again, separation of data.

GetModel() will do additional logic on the reg.Model data that the testing code won't do - or shouldn't need to do - and the logic here shouldn't know about (or do) that extra logic. That's why it should be doing whatever raw GET it needs for testing, and not use reg.GetModel() unless it needs the model with all appropriate additional processing done to it (meaning, the model as seen outside of the model-testing logic)

Same logic for capabilities too, but that's a different issue/PR

That makes sense. Ive removed the model from this step

thirschel-microsoft · 2026-11-02T14:01:36Z

Pretty busy week last week. Ive updated the PR but let me go through and address the comments as well

Signed-off-by: Tyler Hirschel <thirschel@microsoft.com>

duglin

sorry for the delay - busy week.
I think it might be good to move the mutation tests into td_mutations.go (or td_group.go) since I was actually hoping to scope td_registry to just registry-entity testing... but not there yet.

duglin · 2026-11-05T16:24:32Z

 	}

-	nTD.HTTPBodyMustJSON(res, "GET %s", path)
+	nTD.httpBodyMustJSON(res, true, "GET %s", path)


I wonder if it would be better to add a flag to HTTPResponse indicating if it has been logged or not, then LogHTTP(res) could just return immediately if already logged w/o all callers having to know if it's been logged or not and pass in the extra arg?

duglin · 2026-11-05T16:29:39Z

+	createTD.NoError(createRes.Error, "Creating the run-owned Group MUST work")
+	createTD.StopOnFail()


I think you can replace all occurrences of NoError()+StopOnFail() with NoErrorStop()

duglin · 2026-11-05T16:34:58Z

+
+	for _, resourceType := range SortedKeys(groupModel.Resources) {
+		countName := resourceType + "count"
+		td.ObjReqMustGe(res.JSON, countName, 0)


Why do we have the above check given the next one?

Any reason why you're not letting it default the msgs on these types of checks?
I find:

"filescount" (0) MUST = 0

more useful than

file count MUST be zero

since it uses the real attr name and shows "got" and "expected" values:

duglin · 2026-11-06T17:05:45Z

+	epochAny, _ := td.GetObjProp(res.JSON, "epoch")
+	epoch, epochErr := conformMutationUInt(epochAny)
+	if epochErr != nil {
+		td.Log("Invalid epoch: %s", epochErr)
+		td.Fail("\"epoch\" MUST be an exact uinteger less than 2^53")
+	}
+


are these lines needed given the epoch check on line 536?

duglin · 2026-11-06T17:08:24Z

+			"%s count MUST be zero", resourceType)
+
+		if _, exists := td.GetObjProp(res.JSON, resourceType); exists {
+			td.ObjReqMustEq(res.JSON, resourceType, map[string]any{})


I don't think this is true. W/o ?inline they shouldn't show the collection, even if empty. So I think just the MustNotExist() check on line 554 is needed

duglin · 2026-11-06T17:50:56Z

+func TestXRConformMutations(t *testing.T) {
+	const target = "http://localhost:8181"
+
+	t.Run("authorization before network", func(t *testing.T) {


Let's not use t.Run(), just do the test directly. Using t.Run() will cause the golang testing infrastructure to indent the output which makes it really hard to just copy-n-paste the "Raw Output" from the testing output and use it as the expected output. If you look in the Makefile you'll see that there's a "sed" cmd to properly outdent the "go test" output 8 spaces to make this task easier - t.Run() breaks that.

duglin · 2026-11-06T17:57:56Z

+			"http://localhost:8282/conform", "",
+			`PASS: http://localhost:8282/conform (skip:2)
+Pass: 50   Fail: 0   Warn: 0   Skip: 2
+`, "*", 0)


If you search in xr_test.go you'll see uses of ExpectCount func to make this stuff easier. For example, I think this can now be rewritten as:

XCLI(...). ExpectCount("err", "Request: PUT ", 0). ExpectCount("err", "Request: DELETE ", 0)

and it supports regular expressions too. Also, it might be best to look at how it sets up clog to send those HTTP logs to a separate file. Then 1) you don't need -vvv and 2) we can test stderr w/o the clutter of the http logging. Then it becomes ExpectCount(clog, ...) - but it does support "err" (Stderr) or "out" (stdout) if needed.

duglin · 2026-11-06T18:01:43Z

+}
+`)
+
+		XHTTP(t, reg, "PUT", "/dirs/existing", `{


Is this just for fun or is it actually needed for the conformance testing?

duglin · 2026-11-06T20:08:57Z

+	expectedURL := reg.WithPath(path)
+	createTD.HTTPHeaderMustEqual(createRes, "Location",
+		expectedURL, "Create MUST return the exact Location")
+	createTD.StopOnFail()


Did you want to test updating the group (w/ and w/o epoch (good and bad value)) as part of this PR or save it for later?

duglin · 2026-11-06T20:11:44Z

+		deletePath = "/" + groupModel.Plural
+		deleteBody = []byte(fmt.Sprintf(`{%s:{"epoch":%s}}`,
+			strconv.Quote(id), epochString))
+	}


It might be good to test w/o any epoch at all (no query param or in body) - as well as testing cases where ?epoch isn't supported but we try to use it (it should ignore the epoch value - even if it's a bad value). Follow-on PR is fine too - just popped in my head since you're touching on the subject here

Signed-off-by: Tyler Hirschel <thirschel@microsoft.com>

duglin reviewed Oct 20, 2026

View reviewed changes

Comment thread cmds/xr/td_registry.go Resolved

duglin reviewed Oct 20, 2026

View reviewed changes

thirschel-microsoft force-pushed the thirschel-microsoft-conform-group-mutation branch from 888efb7 to dad0cc3 Compare October 20, 2026 19:11

duglin reviewed Oct 21, 2026

View reviewed changes

thirschel-microsoft force-pushed the thirschel-microsoft-conform-group-mutation branch from dad0cc3 to 99ddd74 Compare October 23, 2026 16:28

duglin reviewed Oct 26, 2026

View reviewed changes

duglin requested changes Oct 28, 2026

View reviewed changes

thirschel-microsoft force-pushed the thirschel-microsoft-conform-group-mutation branch from 99ddd74 to 8a8e250 Compare November 2, 2026 14:00

thirschel-microsoft added 5 commits November 5, 2026 19:39

feat(conform): add opt-in group mutation test

bf8adcc

Signed-off-by: Tyler Hirschel <thirschel@microsoft.com>

fix(conform): address mutation review feedback

dbd32c7

Signed-off-by: Tyler Hirschel <thirschel@microsoft.com>

fix: ignore create epochs for all entities

84b3074

Signed-off-by: Tyler Hirschel <thirschel@microsoft.com>

fix(conform): address follow-up review

35592fa

Signed-off-by: Tyler Hirschel <thirschel@microsoft.com>

test(conform): align skip output after rebase

50168ed

Signed-off-by: Tyler Hirschel <thirschel@microsoft.com>

thirschel-microsoft force-pushed the thirschel-microsoft-conform-group-mutation branch from 8a8e250 to 50168ed Compare November 6, 2026 14:36

duglin requested changes Nov 6, 2026

View reviewed changes

thirschel-microsoft added 2 commits November 9, 2026 11:12

fix(conform): address mutation review follow-up

0c509f9

Signed-off-by: Tyler Hirschel <thirschel@microsoft.com>

test(conform): cover mutation eligibility helpers

b192a7f

Signed-off-by: Tyler Hirschel <thirschel@microsoft.com>

		createTD.NoError(createRes.Error, "Creating the run-owned Group MUST work")
		createTD.StopOnFail()

Conversation

thirschel-microsoft commented Oct 20, 2026

Summary

Testing

Uh oh!

duglin commented Oct 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin Oct 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Oct 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

duglin Oct 20, 2026 •

edited

Loading