Some guidance on reproducing, reporting and fixing issues

[areknoster]

Hey! Thank you once again for your effort!

I wanted to give a feedback / suggestions on how to make the project more friendly to early contributors (I myself try to adopt the project, and I'm trying to contribute as well.)

I've found probable bug in the way the form values are handled for buttons.
Long story short, this

    <form id="form" hx-post="/forms/form-1" method="post">
      <input id="field-1" name="field-1" type="text" />
      <button id="submit-btn" name="action" value="submit-btn-clicked" type="submit">
    </form>

would not submit action="submit-btn-clicked" in the form.

Now I wanted to contribute, by adding a test that would reproduce the issue.
I've found following hurdles in doing so:

1. No Contributing readme
2. No documentation on general project structure, how to read that, how to troubleshoot.
   I'd expect that in the top level there are some references to integration / E2E tests that are usually a good place to start - if I could just add some subcase that would showcase my issue, and only then get deeper into the code - that would be great!
3. I've found some tests that I could leverage, but they were written in Ginkgo. I do see Ginkgo working great in a companies or personal codebases, when people are fully commited to the framework and learn their internals. Ginkgo has nice semantics and a lot of tooling. But when it comes to open source, it's a tough sell to expect all contributors to learn that DSL just to write a test or two. I don't know the semantics, I don't know the tooling, I don't know how to contribute to them, and most likely it could be a big blocker for many go programmers to contribute as well. I think there is no coincidence, that all major OSS go projects just stick to stdlib.

I hope this makes it easier to adapt the project and encourage contributors. I think it can bring a great value to go ecosystem.

Best
Arek

[stroiman]

Good points.

I believe that the name should work if the button is replaced with <input type="submit">. But I need to double check.

And yes, good points on overall documentation to help contributors get into the code. Do you have any examples of projects that do this well?

Regarding Ginkgo, I had actually come to the conclusion that it was a mistake, partly for the reasons you mention, that it's a barrier for contributors. So recently, whenever I've been working on a specific feature, I've started by converting the ginkgo suite to non-ginkgo. I was however migrating tests to "testify" test suites, so not pure std lib as you suggest. But testify does seem to be widely adopted, so it should be far less of a barrier of contributors.

I started using ginkgo when I first used Go. Back then the ability to structure tests, e.g., using subtests were limited, and Ginkgo provided clear benefits. But this style of testing doesn't work as well in Go as it does in other languages (few match the elegance of RSpec, to mother of them all). So the other reason I'm abandoning Ginkgo is that with Go subtests, made easily consumable through testify suites, the benefit of Ginkgo just isn't that great anymore.

I don't have plans at the moment to replace Gomega (an assertion library often used with Ginkgo) at the moment, as it allows expressive tests

Expect(document.QuerySelector("div")).To(HaveAttribute("class", "foo"))

I don't find that this should be as much of a barrier, as the intend of this code is very clear. But I will not enforce contributors to use this. As long as behavior is properly tested, I'm good. And simple assertions, such as simple equality or true/false, or error/no-error check, I am also replacing them with testify assertions.

But one nice thing about Gomega is the implicit error return value check. QuerySelector may return an error if the pattern is invalid. So in the previous case, the assertion would fail, of a non-nil error value was returned.

But I'd love to hear your thoughts if you think I should completely abandon the use of testify suites, and Gomega for assertions?

The v8go project uses raw stdlib for tests, and I am personally not happy about not even using an assertion library, a simple assertion takes 3 lines of code, because you need to wrap them in

[areknoster]

As for the reference projects on how to make it friendly from contributor and user perspective, I like
https://github.com/go-chi/chi

As for the testing approach:
To me it's just about making it useful for all possible consumers of the code.
At Datar we prefer stdlib, because it's the easiest to understand to a person without any prior knowledge. Besides, vast majority of our code is generated by LLMs, which happen to also work best with pure stdlib.
Yes, it's less expressive, but it's written only once, and then it's all about reading. Usually when things break. And this is when any implicitness can hurt.
For some more rationale on that approach you can refer to https://google.github.io/styleguide/go/decisions#assertion-libraries
Current setup works fine from my perspective for that matter 👍 The fact that there are some gomega helpers in separate package doesnt hurt me at all.

I have nothing against assertion libraries per-se, as long as they don't obscure too much.

assert.NoError(t, err)

is perfectly clear both to me and to an LLM.

Though when it comes to different test runtimes, like Ginkgo or testify test suite, I'm not a fan for open source. They go beyond standard way of running the tests. They have implicit construction and cleanup. Nice when writing initially, but implicit can hurt when things break, especially when you don't know the codebase too well.
They also don't work well with default go toolchain - how do I run single testify test suite test from VSCode UI, or by using go command?
Do how do I go test -run '<test name>'? And what value do they provide that can't be achieved with explicit function call at the beggining for construction, t.Cleanup for cleanup and t.Run for hierarchy?

[stroiman]

Do how do I go test -run ''? And what value do they provide that can't be achieved with explicit function call at the beggining for construction, t.Cleanup for cleanup and t.Run for hierarchy?

Testify suites do use Go's subtests, so testify suites should play nicely along with IDEs that support Go's subtests, and also works with the -run option - but considering the syntax necessary when specifying subtests, having to add a / in the spec, which I find a little odd.

But I appreciate the input, and I might eventually migrate to naked Go tests eventually.

For the time being, I thing that the migration, at least for bigger tests will be a two-step process. Ginkgo->testify suites before eventually going naked, as there's a lot of shared setup, and first migrating to testify suites, I can defer the problem of extracting common setup code.

[stroiman]

But for the concrete issue, I will start refactoring tests regarding form behavior. If you would like to add the test for the issue, let me know. Then I'll just stop at the point when the tests are refactored.

Btw, the main branch is currently in a breaking change state. I'm completely refactoring how events work, but I'll give more details in a different "discussion". But I'll just create a fix-branch for this issue.

[stroiman]

Oh, interesting. I don't think that that test should be skipped. I will investigate.

But for the specific bug, that is form handling behaviour itself, and not related to JavaScript, so I'd test that directly on the form, which is html/html_form_element_test.go.

		Describe("ReqeustSubmit with a <input type='submit'>", func() {
			var submitter dom.Element
			BeforeEach(func() {
				submitter = window.Document().CreateElement("input")
				submitter.SetAttribute("type", "submit")
				submitter.SetAttribute("name", "submitter")
				form.AppendChild(submitter)
			})

			It("Should add the name of a submitter, if passed", func() {
				form.RequestSubmit(submitter)
				Expect(submittedForm).To(HaveKey("submitter"))
			})

			It("Should ignore the name of a submitter if not passed", func() {
				form.RequestSubmit(nil)
				Expect(submittedForm).ToNot(HaveKey("submitter"))
			})
		})

[stroiman]

As mentioned, it was tested with <input type="submit"> as I suspected, but not <button>

[stroiman]

But just looking at the code, I'm puzzled why the name doesn't work. Not sure it's a <input> vs. <button> element.

[areknoster]

Yes, doesnt seem like. I thought that's the issue because that's what I was working with. I've made a commit that showcases the issue
#52

[stroiman]

Awesome, I'll take a look.

[stroiman]

And thanks for feedback.

I love to hear that this is being used, and the feedback is also extremely important in understanding the direction to take, and what to prioritize.

And to keep me motivated.

[stroiman]

Correction, the main branch shouldn't be breaking in itself., I didn't push the breaking changes to that branch. But it does include additions that would be affected by the breaking change, so I'd still prefer to not release it as is.

[stroiman]

Allright, I've identified the issue in your repro, and that's a bit embarrassing.

So first of all, thanks for providing a repro.

The Content-Type header was missing on the request, causing the server to not parse the form data 🙈

Tests were passing because they verified the request body, and not the result of req.ParseForm - so they didn't catch that the form was invalid - and the test that did use the req.Form was a GET request, not a POST.

It's pushed as v0.3.1

HOWEVER

My test application didn't exhibit any issues, and I believe that was because it was an HTMX post, so the bug in Gost's form post behaviour didn't affect that case, because it creates the headers on an XHR request.

So while the patch fixes the case you made, if shouldn't apply to the original problem, as it uses hx-post (assuming you actually have HTMX loaded. Do you serve it from static resources locally? CDN doesn't work when using the browser.NewFromHandler option)

    <form id="form" hx-post="/forms/form-1" method="post">
      <input id="field-1" name="field-1" type="text" />
      <button id="submit-btn" name="action" value="submit-btn-clicked" type="submit">
    </form>