boydm / phoenix_integration

Lightweight server side integration test tools for Phoenix
Apache License 2.0
219 stars 26 forks source link

Tests suggesting different handling of checkboxes (#33) #34

Closed marick closed 4 years ago

boydm commented 4 years ago

Hi @marick. This is getting there! The build is currently throwing a bunch of warnings for unused aliases and an unused attribute (@form_id). The tests are also putting out what looks like debug spew. Could you please clean those up and then I think we are getting close to integration.

boydm commented 4 years ago

And I both recognize and appreciate the large amount of work you've done here! I really appreciate it.

marick commented 4 years ago

Yes, that was an unfinished version. Today I did work to make the new code (build_form_data__2) do everything the existing code does (plus checkboxes and also multi_select). I think that's done. The only difference I know of is in the error message printed when a submit_form's user edits refer to something not in the form. It looks like this:

Screen Shot 2020-02-09 at 5 25 51 PM

That's probably too much spewage, but I wanted to stick close to what you already have. Probably it's enough to just print something like <form accept-charset="UTF-8" action="/form" method="post" id="proper_form">

So I think this is almost finished, save for maybe working to produce more errors -- and even warnings about forms that can't possibly do what was intended. (Because such information is readily available.) Depends on how much hand-holding you think users deserve.

marick commented 4 years ago

P.S. I also haven't implemented anything to keep error messages from contaminating test output. That would be another part of cleanup.

boydm commented 4 years ago

Safe to assume the users need more hand-holding than we can imagine. ;-)

This great stuff.

marick commented 4 years ago

Here's what I did this afternoon.

Screen Shot 2020-02-10 at 6 39 04 PM

Any comments?

Also: I tried using IO.ANSI.default_background to keep the last color from infecting everything that comes after. As you can see above, it doesn't work. Do you know anything that does?

marick commented 4 years ago

Forget the question. A good(?) thing about having lots of twitter followers is that I can get corrections to stupid mistakes really fast.

Screen Shot 2020-02-10 at 6 57 32 PM
boydm commented 4 years ago

That's funny.

And you can always set the color to the Ansi reset character

boydm commented 4 years ago

This looks like it is really getting there. Let me know when you think it is "done" and then I'll really go through it.

marick commented 4 years ago

It's feature complete. I'm going to check any documentation needs changing, and go over the code one last time to tidy it up.

Do you have any test suites you can easily use it with?

boydm commented 4 years ago

Still one skipped test. I think maybe we should just remove it? Especially if we can't actually tell what was set to false.

boydm commented 4 years ago

Also, this is a big enough change, I'm going to call it 0.8.0. I think after we do the merge, I'm going to update the versions in master, then lets both tweet (or whatever) that this is coming to give anyone interested a chance to try it out before pushing to hex. I'm thinking maybe a week of that.

What do you think?

marick commented 4 years ago

I've deleted the test. I agree about the versioning.

I've finished my cleanup. From my end, this is ready to go.

marick commented 4 years ago

Also maybe https://elixirstatus.com/

boydm commented 4 years ago

This really is a great piece of work. Thank you very much. I'm going to merge it in, update the version, then put a call for people to try it out.

marick commented 4 years ago

Aw shucks. Thanks.

boydm commented 4 years ago

Just published 0.8.0 to hex.