Bug 1/30: Dotnet CLI Parsing
Bug 1 is dotnet/command-line-api/Trailing spaces break parsing in a response file. It looked straightforward, so I thought it would be a good one to start with.
Apparently the dotnet CLI has a built in file parsing mode that allows the user to supply a file containing the program’s arguments instead of supplying on the command line itself. This file has 2 modes:
- Arguments / options delimited by a line break.
- Arguments / options delimited by whitespace.
The expected behaviour described in the bug is that in line break mode leading and trailing whitespace should be ignored on each line, however this is not happening.
When working on a bug on GitHub the workflow is generally as follows:
- Fork the repository on GitHub.
- Clone the fork locally.
- Create a local branch to work on the bug.
- Fix the bug in the branch.
- Push the branch to your fork on GitHub.
- Submit a pull request.
- Repeat steps 4. to 6. until your pull request is accepted.
Fixing the Bug
Whenever I get a new repository the first thing I do is make sure I can build it and run all of the tests. In a well designed C# solution this is usually a simple matter of loading the solution in Visual Studio, hitting F51 and then running the tests in the test explorer. Having spent years programming C++ in a text editor and debugging on the command line with std out I never cease to marvel at just how easy Microsoft have made software development.
In an ideal world there are three parts to fixing a bug:
- Write one or more failing tests that assert the expected behaviour.
- Find the parts of the codebase where the failure occurs.
- Get the tests to pass.
The only slight hurdle I encountered when building the solution was that it targeted a version of the framework (4.6.2) that I didn’t have installed. Having installed this everything built without warnings and all of the tests passed first time. So that was easy.
I searched for the word
responsefile in the entire solution, as this is how the feature is referred to in the bug. I found two useful classes:
ResponseFileHandling: an enum that describes 3 different modes (line bread / whitespace / disabled).
ResponseFileTests: the test class containing response file test.
I looked over the other tests in
ResponseFileTests to get an idea of the style and wrote a failing test that covered what I thought was the important behaviour in that style.2
After trying to step through the test for a few minutes I realised that I would be better off trying to work out where the bug might be occurring without the debugger, as I could see there was going to be a lot of code to step through and I would likely miss the errant line through boredom. Luckily the
ResponseFileHandling enum came to my rescue. I spotted the line
in the find window where it was immediately obvious that the wrong variable was being returned. The original developer was trimming the argument at the start of the method, but yield returning the untrimmed argument. I made the change and the test passed.
The PR can be found here.
Not particularly interesting, however a good intro to this particular codebase. In all it took me less than half an hour, which has left plenty of time to write this up and get to the gym.
As I predicted I was asked to make a small change; split the test into three separate tests. A very sensible suggestion.
One down, 29 to go 😄
Yes that is why the site is named as it is. ↩︎
After submitting my pull request I realised that the name I’d given the test was not entirely accurate, however in my experience most reviewers like to suggest at least one change whether one is necessary or not, so I thought it would be better not to modify it unless asked. ↩︎