Day 4: Playing whack-a-mole
Today, I felt like I was forced to play whack-a-mole:
Everything started nicely: Someone posted a pull request to handle links with (non-standard) spaces correctly with hints (<a href=" http://example.com ">), I told them how to add a test, they did, I fixed a small issue, bam, merged.
Then I fixed some tests which were failing due to changes from yesterday. The first one was a test which failed reliably on Windows since changing the test to use a real file instead of a mock.
It tests a function getting the last n bytes from a file, with 64 bytes. On Windows, it expected:
58 new data 59 new data 60 new data 61 new data 62 new data 63
ew data 59 new data 60 new data 61 new data 62 new data 63
Unfortunately pytest failed to produce a nice readable diff like it usually does:
> assert lineparser.get_recent(size) == data E assert ['ew data 59\...ew data 63\n'] == [' 58\n', 'new...ew data 63\n'] E At index 0 diff: 'ew data 59\n' != ' 58\n' E Right contains more items, first extra item: 'new data 63\n'
After printing the raw values staring at them for some seconds, I figured the few missing bytes were exactly the count of \r (carriage returns) you'd need to insert.
While Python takes care of the conversion when reading/writing files, when getting the last 64 bytes of a file, you'll get less data on Windows.
I fixed the code to use os.linesep instead of \n, and it still was off by one on Windows, but not in Linux:
# data = '\n'.join(self.BASE_DATA + new_data) data = os.linesep.join(self.BASE_DATA + new_data) data = [e + '\n' for e in data[-(size - 1):].splitlines()]
I then figured out I actually had an off-by-one error there - the "-(size - 1)" should actually be just -size. What I actually was missing is a "+ os.linesep" for the final line ending. I guess when originally implemented it I thought I just was off by one while slicing and naively fixed it the wrong way...
With that out of the way, I looked at the other test which was flaky - it reads the history a qutebrowser subprocess, definitely waits until the subprocess has written it, and still sometimes ends up with an empty file.
I let the logs sink in for a bit, but I still have no idea what'd cause it. In the end I just ended up marking the test as flaky using pytest-rerunfailures. This means it'll be run a second time if it fails, and if it passes then, it's assumed to be okay.
This is definitely less than ideal, but it's better than having a test which fails sometimes for no apparent reason, and better than not testing this functionality at all.
After all that, I updated to Qt 5.6.1 to check if a segfault on exit was fixed like claimed in the bug report.
Turns out it wasn't, but instead there was a new bogus warning and a weird behavior change I needed to take care of in my testsuite.
Now I hoped I was finally done fixing weird bugs - turns out that was just the beginning. Someone joined the IRC channel and reported how hints often don't work at all for them since the big hint changes from Monday.
I couldn't reproduce the issue, and what we were seeing made no sense at all. See the bug report I opened to see all the gory details. If I tried to write them up here, I'd probably just hopelessly confuse myself again.
They are an experienced Python programmer as well, and after over 3.5h of debugging, we gave up.
I ended up adding a setting which allows to revert back to the old Python implementation. It's less accurate but also faster than the new (JS) one, so some people might prefer that anyways.
And some other changes
Before and after that frustrating debugging session, I also managed to get some other changes in:
I improved the error message when an invalid link is clicked as I stumbled upon it and it confused me.
I also started refactoring the history implementation and adding a few tests to it, as I still need to do a small change to handle redirects nicely before releasing v0.7.0 (what is what I originally planned to do today...).
The refactoring also allowed me to split off the QtWebKit-specific part of it nicely, so that's already a little step closer to QtWebEngine as a nice side effect!
The todo list for tomorrow roughly looks like this:
- Handle redirects in saved history
- Merge the trivial doc PR for the Debian packages
- Package and release v0.7.0
I really want to release v0.7.0 tomorrow unless another serious regression is found (fingers crossed!).
And then full-speed towards QtWebEngine support next week!