Thread by @frioux

One of my day-to-day programming superpower tools is pup (github.com/ericchiang/pup). Pup let's you easily pick apart html on the commandline. If you are familiar with jq, it's like that, but uses a standard selector language (CSS).

Anyway, I thought I might have found a bug and decided to look at the project. It's been reliable for me, but has had no changes in 3 years and no release in 8 years. I figured I'd try my hand and doing a cleanup pass.

So first I forked it to github.com/frioux/pup and changed the readme slightly to clarify what this project is for.

Next I did a simple `go get -u` to update the few deps. At the same time I told it to build with go 1.17 instead of go 1.13.

I would like to keep these deps up to date, so I enabled dependabot, github's tool that sends you PRs when your deps have new releases: github.com/frioux/pup/com…

I also added some basic CICD so that new binaries would get created automatically. Aside from a mistake around go module vendoring, it worked fine on the first try: github.com/frioux/pup/com…

The code has some hardcoded versioning. I dislike that as it's too easy to leave the old version in place even after changes are made (like the current upstream.) I removed it because the Go release in a month or 2 will automatically add version metadata.

Next, I noticed that the code has a set of tests that are not run by `go test` in the tests directory. It uses python to run pup with a committed html file and asserts that the output matches a given sha.

The annoying thing about this is that if the tests fail, you have no idea why or what's wrong, you just know "something changed". My friend @mmcclimon joked that maybe I should just reverse the shas to see what changed.

To fix this, my intention is to replace the python + shell test with table driven go tests where the output is checked against files checked into the repo. We can generate those files by passing a flag to the test.

Unfortunately, when I went to add such a test, I found that pup has a bunch of globals and a large-ish main that is not amenable to these kinds of tests!

While it'd be nice to refactor first, I'd rather add the tests with the globals in place, then refactor with the help of tests.

As much as I'd like to change zero code before writing the tests, I did create a function out of this code github.com/frioux/pup/blo….

In my tests I reset all of the globals before running the function that parses commandline flags: gist.github.com/frioux/afef8d4…. So far this works, but I need to figure out how to capture standard out.

I also did a weird hack to allow the current arg parser to only ever look at os.Args: gist.github.com/frioux/afef8d4…. I wanted to do a similar thing to capture standard out but os.Stdout is an *os.File so I can't just set it to some other writer.

To capture standard out I have a few options. I could add yet another global and have all the various displayers (github.com/frioux/pup/blo…) write to that. I could also add a required function to the Displayer interface that lets you set the writer and then set that.

I could also add a writer to just the displayers that end up getting tested and do a type switch on the global displayer to be able to set those.

My hunch is that just adding a SetWriter to Displayer (or more likely just changing displayer from Display([]*html.Node) to Display(io.Writer, []*html.Node) would be best. (Gonna take a break now.)

OK, took my kid to the park, took a jog to the beach. Gonna try to see if I can wrap this up this afternoon.

I successfully added io.Writer to the Displayer interface: github.com/frioux/pup/com…. Now to finally (🤞) port these tests over.

Before I do anything, I should call my shot: there are failing tests in the official repo, and because they fail with a "sha DEADBEEF != sha CAFEBBABE" I am probably not going to bend over backwards to figure out what's wrong unless it's pretty obvious.

Anyway here goes. Now that the stage is set I want to iterate fast, so I open another terminal and run gotest (github.com/frioux/dotfile…) which automatically runs tests whenever files change.

Annoyingly a lot of these tests produce no output even with the original pup. I started going down a rabbithole to figure out why but instead I'm going to try to wrap up the testing infrastructure.

OK I got the testing stuff to work. It correctly errors if I change the golden files, but an... unlikely amount of the tests are producing blank output. I think temporarily I'm going to call blank output a failure.

These tests have blank output: 2, 3, 4, 15, 34, 35, 37, 38, 39, 47, 48. Here are the tests: gist.github.com/frioux/afef8d4…. I mark each broken test with a `// XXX` comment so I can easily see the broken ones.

Here's the pattern I see: all the tests with a + are broken. One is not reporting as broken, but that's because it's reporting output as [], which is still blank, but not zero length (it's JSON.) The tests with > are also failing. And finally, the two tests with flags fail.

The question for a lot of these is: are the tests broken or did I somehow mess up my testing infrastructure? To be sure, I'll add an optional (and maybe temporary) test that will compare my results against the original version of pup.

Good news: the last two tests were broken due to how I'd tokenized args (by hand.) The rest seem broken even in original pup. Fun!

Ok here's a comical bit of evidence. The original test data: github.com/frioux/pup/blo… . Note that da39a3ee5e6b4b0d3255bfef95601890afd80709 shows up suspiciously frequently. Punchline:

$ echo -n "" | sha1sum
da39a3ee5e6b4b0d3255bfef95601890afd80709 -

So there are a bunch of broken bits in the original code, subtly so because the tests don't clearly show their expected output! Tomorrow I'll wrap up marking these tests as TODO or something and then see about actually trying to fix these issues.

Side note: some of the tests in the original were broken but not with mine because the original passes all the args as a single string, rather than allowing pretokenization. That's cool I guess!

Wrapping up test infra. Before:
$ ./test
13c13
< ecb542a30fc75c71a0c6380692cbbc4266ccbce4 json{}
---
> 199188dc8f1522426a628e41d96264bffb8beb0f json{}

After:
pup_test.go:113: testing args: [table a[title="The Practice of Programming"] text{}]
pup_test.go:149: test does not match golden data (-got, +expect):   string(
- "Il Practico de Programming\n",
+ "The Practice of Programming\n",

I really love the ability to add flags to tests for uses cases like this: github.com/frioux/pup/blo… (see github.com/frioux/pup/blo… for relevant code.)

h/t to @mitchellh for his excellent presentation that shared this pattern:

I'm timeboxing efforts to actually fix this code. First off, the + combinator. Here are the relevant MDN docs: developer.mozilla.org/en-US/docs/Web…. Most of the tests for + in pup start with li (like `li + a`.)

I would never have an `a` adjacent to an `li`, since the `li` would be fully enclosed in a `ul` or `ol`. Anyway, the sample HTML follows my assumed rules, and changing the tests makes it clear + does work, but the sample selectors find nothing.

OK I replaced all of the + tests with working versions. At this point my expectation is that the other failing tests will be similar.

Confirmed for >. The only tests for > (docs here: developer.mozilla.org/en-US/docs/Web…) are `li > li`, which, I believe, no one would ever reasonably do. When I replace them with more sensible selectors I find expected results. Fixed those.

Last one is .after. There is no use of the after class, so this is just... nothing. I fixed tests to use a real class, and finally removed the skip support.

Next I need to merge in some outstanding PRs! How about these:
* github.com/ericchiang/pup…
* github.com/ericchiang/pup…
* github.com/ericchiang/pup…
* github.com/ericchiang/pup…
* github.com/ericchiang/pup…
* github.com/ericchiang/pup…
* github.com/ericchiang/pup…

#121 (support for *) is the easiest, so I pull it in and reimplement the tests. Next, #107 (support for unknown element types) seems really useful and pretty simple too, so I pull that in (and add tests.)

#158 (support for -n+A in :nth-child) is a little sloppy. Seems ok but let's come back to it. #139 (multiple attribute matches for the same attribute) seems pretty niche. The code is fine, but again, let's keep going and come back to it.

#111 seems sensible, but it breaks backcompat and is pretty obscure. If I pull it in it'll be as a separate Displayer (like `json2{}` or something). Same with #109, maybe they should both go to the same Displayer?

#81 is fascinating (like grep -v but for pup.) It merged in really easily. The one issue (so far) is that it doesn't compose with * (ie -v * still produces everything instead of nothing.) OTOH for all the actual sensible selectors it works fine.

This is a good start. I'll see about merging in the rest some other time, in some other thread.