As I was preparing for my finals and fixing some automation tests for my Pull Request (PR), I realized I had made a serious mistake. I was clearly under no influence of alcohol, but I managed to write this comment:
Comment for
#20494
That's strange. The tests actually passed when I ran them locally. Should I try to fix it, @timabbott? Sorry, I forgot to run the tests for the whole codebase.
Edited: I have no idea how to approach this, so it might take some more days
Edited 2: I think I know why now. I removed the use of make_query_highlighter() in search_suggestion.js. It became obsolete since it was used there only. If I'm not mistaken, it was used to make every matching character of a suggestion bold. In this case, it's the full name of a user. Because the name is inside a pill now, I think the bold text doesn't stand out a lot like it used to. Should I add the feature back?
What happened
I'd like give you some background information first. Zulip strives to maintain 100% code coverage. In order for my PR to be merged, it must meet that requirement. If the code coverage is compromised, this message will be displayed:
Believing that getting all my tests passed was enough, my PR failed the CI check at first. Unfortunately, that wasn't the worst part.
Thanks to the test coverage, I noticed there was an unused function. It was used by some functions that I'd modified. This was a huge red flag, but it took me some time to realize that I removed an existing feature without asking the maintainers 🤦. Specifically, the feature highlights any substring in a suggestion that matches their query.
In my defense, there was a complication with integrating the new feature (i.e. showing avatar and username in pill-shaped UI elements) with the one mentioned above. I quickly discussed the issue with the maintainers.
My first attempt at solving it failed. Forgetting the test my code thoroughly, I made a silly mistake by double-escaping HTML expressions in every user's name:
Comment for
#20494
Sure, @alya! Here you go:
... and thanks to you, I found something that looks a bit off. Right now, ' is currently considered a dangerous character (replaced by HTML entity '). Currently, it seems that we allow that character to be in a full name, so let me try to make a patch for it. Is there anything else I should change?
Thankfully, I managed to patch my mistake instantly. Here's how it looks:
What I've learnt
Thanks to the test coverage report, I managed to discover my embarrassing flaw. Although it may be annoying to update the tests at times, I think I've learnt to appreciate a good CI pipeline setup more than ever. I found Zulip's docs a good resource for how a developer should approach testing.
I've also recognized the importance of sleep to keep my sanity. Looking back on my comments and code, I'm astonished by my audacity to have made such mistakes. I should have got started earlier to allow myself more time to get familiar with Zulip's codebase.
What's next
Since my PR is still under review by the time I'm writing this blog, I'll try to improve it until it's merged. There are not (hopefully) a lot of things left to do with it, so it should be done soon.
Cover photo by Brooke Cagle on Unsplash
Top comments (0)