Working on a DW Bug: Step by Step

[crschmidt]

So, when I woke up this morning, in #dreamwidth, there was a discussion of a bug in auto-expanding lj-cuts in a particular entry. It looked interesting enough that I figured I would poke at it, since there was a theory it was a Javascript bug, and Javascript is a language I speak. (The actual bug was that clicking on a particular lj-cut expansion would expand to a different lj-cut text, now filed as Bug 4435.)

First, I didn't even know how to expand LJ-cuts inline. So I started from pretty much nothing here. :) But in the end, I think I found the bug, and attached a patch to Zilla, so I figured I'd write up my process.

Start: (6:46) Okay, clicking on the cut gives the wrong thing. Open up my browser's web console, and check the HTTP request that it's sending is right -- it specifies cutid=1, but is given one from a much later cut. (6:48) To me, this suggests that the problem is server side (not Javascript), so I have to dive into code I can't just load in my browser. I experiment a bit more to ensure that this appears to be the case, and determine I can't see any evidence that this is client side. (6:58).

I Googled 'dreamwidth code' (which didn't get me very far), then 'dreamwidth mercurial', which got me to the hg repo (even though it was the 5th result) first. After realizing I couldn't figure out how to check out the code from there, I went back to the first hit for that search, the opensource page, and made my way through to the wiki and from there to Getting Started and from there to Scratch Installation and from there to Downloading the code from which I grabbed bootstrap.pl, and from there to the one line I wanted: 'hg -q clone http://hg.dwscoalition.org/dw-free'. (Yes, if I knew mercurial better, I probably could have done this without following any of those steps ... but I didn't. In fact, I didn't even have mercurial installed at the time. So I also installed that.)

So, it's now 7:04, and after grep -r-ing for rpc_cuttag, I find cgi-bin/DW/Controller/RPC/CutExpander.pm, and realize that all the work in the cut expander is done by LJ::CleanHTML. My memory of working on LJ almost a decade ago was that this particular chunk of code was a hairy, nasty, mess -- as is almost any code that parses HTML -- so I was unsurprised to find that little has changed ;)

After looking briefly at CleanHTML (07:11), I decided that this was almost certainly a side effect of the particular entry text -- simple tests showed that this worked, but this entry was clearly failing. I checked briefly whether any community mods for that community might be in #dreamwidth -- the answer to which was no. (07:12).

At 07:15 I summarize my progress so far to the channel:

1. Javascript is doing fine. IT's asking for the right cut. The server side is broken.
2. The server side RPC mechanism is likely doing the right thing; there's no smarts there.
3. Once it gets past that, it goes into the CleanHTML code, which in addition to being the gnarliest perl code I've ever not touched, definitely behaves very differently when in "looking for cut mode"
   than it does otherwise, so it's not super-surprising that this fails.

At 07:19, I basically declare defeat: without the source text for the bug, it will be very difficult to debug, but I continue to read the CleanHTML source anyway, trying to understand it slightly better. It's clear that lj-cut handling isn't trivial. I find a couple interesting things in the code, but nothing that calls to me as the source of the bug.

Around 07:40, I realize that it is possible for tags like <div class="ljcut"> to be turned into real cuts -- presumably to support the rich text editor -- and that these are handled like lj-cuts in one chunk of the code, but notably *not* in the chunk of code that counts lj-cut tags in cases where we are looking to expand cut text.

At 07:42, I reproduce the problem that the entry demonstrates in my own journal, so getting content from the original author is no longer important. There's now enough information to file a bug about this issue, so I search zilla for anything related to lj-cut and auto-expansion, and find nothing. I spend the next 15 minutes writing up the bug, bug 4435, which I finish at 07:57.

At this point, I start to poke at the code and figure out if I can actually fix the issue.

In short, the problem is that two different types of lj-cuts are treated differently by different sections of the code. The fix is to change that. The code for these is relatively simple. In short, I started going through the entire section of code looking for things that explicitly mentioned lj-cut, and trying to update those sections to take into account *both* types of lj-cut.

So (08:01), I uploaded an initial patch, but there was one lj-cut I wasn't sure if it should be handled or not, so I left a comment to that extent in the patch. I then investigated that branch -- which, specifically, was the code that puts a literal </cut> in the view that community moderators see. (This chunk of code won't display the end-cut text in cases where a user uses an RTE-inserted div-class-ljcut element.) I uploaded a new patch with a comment about why I wasn't touching that code.

I then went to my Dreamhack, which Sophie set up for me last week (Thanks Sophie!), and ran the start-apache and followed the "Create a system user" item in rah's wikipage. I then applied my patch to the CleanHTML code, and created an entry under the system account. (08:17)

Then I got confused briefly, because the code... worked on my LJ install. Then I realized that the CleanHTML perl code isn't read when Apache starts (when it was broken) but instead when it is needed (which wasn't until I opened the entry, after which I had already patched it). I reverse-applied my patch, restarted apache, and confirmed the code failed. (08:22)

On IRC, V_PauAmma_V then helped point me to the tests, under dw/t/ on my dreamhack. I couldn't actually figure out how to run them, but did realize they were Perl, and ran them that way. (Note that running tests is not actually described on any Dreamwidth wiki page I can find; I checked submitting your changes to dreamwidth, dev patches, dev version control and some others, and didn't find any place where it said how to run them.) She then pointed out that they were runnable via `prove`, so I used that.

At 08:32, I had failing tests for the problem that I was trying to fix. I applied my patch, and re-ran the tests, and they passed (08:33).

I tried to prepare a patch, but dw/t/ and dw/cgi-bin/LJ/ weren't under hg. I was going to just copy the changes back to my local machine, but instead I did `find . -name .hg` in my homedir, and found that the hg repos are under cvs/. So I copied my changes into cvs/dw-free/, and generated a diff from there. While I was doing that, I ran the tests across the entire codebase using 'prove' in the dw/ directory. (08:43).

I uploaded the patch to Bugzilla, marking it review?; V_PauAmma_V suggested I also mark it commit?. (Took me a bit to figure out how to do that, but I got it.) I then also generated a patch in hg-changeset format, in case that was preferred, and added it to the ticket. (Note that there are also no docs that I could find about how to generate a patch; there are a couple places that do say "Make a patch.", but nothing that says how.)

Total time: ~2 hours, start to finish, with a lot of stumbling around to figure out how things were supposed to work, and I'm still not particularly sure that I did things right, but it does look like I actually fixed the problem... the question is really just whether it also made anything else worse :)

Comments

[exor674]

For the record in the future, changeset patches are not preferred over non-changeset patches -- What type of patch is uploaded just depends on which type of workflow the dev is using.

Also props on braving CleanHTML on your first patch. ( I hate hate hate hate hate hate CleanHTML )

[crschmidt.net]

Yeah, I remembered CleanHTML as being a particularly evil chunk of code from a decade ago -- looks like my memory wasn't wrong, nor have things particularly changed -- beyond the fact that I can actually read the code now, which back then probably would have been well beyond me. :)

[brainwane]

Thanks for writing this! It was informative to read.

[crschmidt.net]

No problem. I really should have included a bit more of the mechanics, but I wrote it in part because I'm working with a new developer to a couple projects, and wanted to do a comparison for time with his development process: He's convinced that he works 'very slowly', and I wanted to point out that the minimum time to fix any bug in any software is typically about 8 times as long as it takes to actually write the code to fix the problem :)

[denise]

Thank you for writing this up, by the way! I love it when people do stuff like this to show their thinking and the approaches they took.

(Also, if you can think of better ways to document things -- including running tests, etc -- please do update the walkthroughs/documentation bits on the wiki! I think there's some posts about running tests in dw_dev_training somewhere, but that's definitely something that should be more well documented.)

[foxfirefey]

This is the page we have so far on testing, definitely could need some expansion:

http://wiki.dwscoalition.org/wiki/index.php/Dev_Testing

crschmidt--if you contact me on IRC we can get you a wiki account. I'm so sorry account registration is akimbo right now; I upgraded the wiki and found myself in an awkward situation and have to figure out how to allow registrations and avoid all the spams.

[crschmidt.net]

I think that there are quite a few mechanics-related things missing: specifically, I think that:

• The fact that there are automated tests isn't documented anywhere that I could find. I think that it probably makes sense:
  1. as a last section in 'start programming (http://wiki.dwscoalition.org/wiki/index.php/Beginning_dev_checklist#Start_programming)
  2. There's a "make a patch" in the Dev patches (http://wiki.dwscoalition.org/wiki/index.php/Dev_Patches) wiki page -- the 'To figure out how to make a patchfile, see Dev Version Control.' text should be moved up to be next to bullet #4 directly above it (and probably a link to Dev Testing (http://wiki.dwscoalition.org/wiki/index.php/Dev_Testing) above it.
• Couldn't find a single line that just told me how to check out the code. I realize that for most things, you actually want to have a full setup, which is best through tools, but a quick 'how to get the code' link would have been nice for the "Is this really a bug" use case. (Other comments in threads in this community make me think that other new developers might benefit from this as well, though most of them are in a state where they'd probably like to be working on dreamhacks.)

Still, overall this was a much better experience than any other open source project I've touched of this size. There is clearly an intent to make development possible -- even for very very new programmers -- which works pretty well, compared to what I would expect, so the Dreamwidth dev community deserves a pretty nice set of props for that.

[momijizukamori]

Awesome to see a walkthrough like this (so far I've only done S2 work, but I'm trying to psyche myself up for Perl). And thank you for the reminder that the patch guidelines are kind of... not there. I ended up having to ask someone in IRC how to do it, but I meant to go back and add that info to the talk pages of the wiki, at least. Along with my 'intro to Mercurial Queues' write-up.

[azurelunatic]

Thank you for writing this up.

I added the Mercurial repository location to http://openhatch.org/+projects/Dreamwidth because that's definitely an important thing.