This week for my open source contribution I decided to tackle a bigger project and a slightly more complex issue. Last week, I removed commented code, this week I added some code. For my repository, I chose to fork Jasmine, and the issue involved adding some extra descriptions to an error message regarding "expectations" and asynchronous tasks in the Jasmine framework.
The issue was that the error message was pointing to one cause, but not anything else that was possibly causing the error. I had a look at the previous comments about what they were doing when the error occurred, and some suggestions from a contributor about done() function and how it executes.
In order to find the file to change in this massive project I took advantage of a Github feature. It allows you to click on a function (similar to Visual Studio) and find the definition. I clicked the error generator function to see where it was defined and change the error.
He mentioned that done() should only be called when you're sure the test is finished and that there won't be any async tasks started after done(). Originally the issue was reported when someone did in fact call done() and then start an expectation.
To fix this, I amended the error message in 2 different files (env.js and EnvSpec.js) to include 3 potential causes:
- 1. Did you forget to return or await the result of expectAsync?
- 2. Was done() invoked before an async operation completed?
- 3. Did an expectation follow a call to done()?
In this way the person running the test can check more causes than just awaiting the result (such as terminating the test before the task can finish).
Each time I made a change, I ran "npm test" which runs their full test suite to make sure that all the errors and test cases pass.
For example, the test caught me when I didn't change the error in env.js but I did change it in EnvSpec.js and it gave me a message saying these 2 errors don't match each other even though they're from the same cause.
Before I could submit my Pull Request however, I had to fix some styling issues such as running Prettier. Furthermore, the maintainer of the project asked that I make my lines of code no more than 80 columns. To fix this, I split the string into separate lines using concatenation and newlines. Finally, in order to merge I ran "git rebase main -i" and squashed my 4 commits into 1. After those steps my pull request was accepted!!!
This was my first time working on such a large project and repository and it was a unique experience. One thing I would do different is add even more description to my pull request about how I tested the feature and what I changed.
Top comments (0)