Feature/add unit testing framework and start writing unit tests (acceptance tests) #708
Conversation
…stead of just executing it Otherwise this code is executed everytime state.js is imported somewhere in a file. This leads to not being able to test properly, since state.js is imported in a lot of the src files and fails when running the tests since we are missing global environment settings such as document, window etc in the jest environment.
| let mutationObserver = new Object(); | ||
|
|
||
| if (isShorts() && mutationObserver.exists !== true) { | ||
| cLog("initializing mutation observer"); | ||
| mutationObserver.options = { | ||
| childList: false, | ||
| attributes: true, | ||
| subtree: false, | ||
| }; | ||
| mutationObserver.exists = true; | ||
| mutationObserver.observer = new MutationObserver(function ( | ||
| mutationList, | ||
| observer | ||
| ) { | ||
| mutationList.forEach((mutation) => { | ||
| if ( | ||
| mutation.type === "attributes" && | ||
| mutation.target.nodeName === "TP-YT-PAPER-BUTTON" && | ||
| mutation.target.id === "button" | ||
| ) { | ||
| // cLog('Short thumb button status changed'); | ||
| if (mutation.target.getAttribute("aria-pressed") === "true") { | ||
| mutation.target.style.color = | ||
| mutation.target.parentElement.parentElement.id === "like-button" | ||
| ? getColorFromTheme(true) | ||
| : getColorFromTheme(false); | ||
| } else { | ||
| mutation.target.style.color = "unset"; |
There was a problem hiding this comment.
This was the one place where I had to change existing code, to make testing possible.
The problem was, that this code was always executed whenever state.js was imported somewhere. But because in there are variables that were not existing in the test environment, there were errors all around the place.
I moved to code to it's own function initMutationObserver() and exported it, to be used in the one place where it should be called. That's what we should aim for when using modules in the future. Loose code otherwise will lead to problems.
There was a problem hiding this comment.
Noted. Mutation Observer was a new feature introduced in the last code merge. It was very much needed as to fix several bugs and adapt to the new and weird Youtube behaviors. Will fix soon. Thanks.
There was a problem hiding this comment.
No worries, I think I fixed it without removing behaviour by putting it into it's own function, but feel free to have a look and verify that :)
| roundDown, | ||
| numberFormat, | ||
| getNumberFormatter, |
There was a problem hiding this comment.
I exported those as well to make them testable
There was a problem hiding this comment.
If they need to be exported by you to complete the test, does that mean it's better to do it in the source code?
There was a problem hiding this comment.
What exactly do you mean by
does that mean it's better to do it in the source code?
But one thing to add: Typically if something is not exported, it means it's an implementation detail and should not be tested separately, but rather by testing the public/exported functions that use them. But in this case I decided to make them testable since the exported function is a rather big unit of code. But i someone disagrees we can always undo that and remove the tests for roundDown and getNumberFormatter again.
There was a problem hiding this comment.
Is it better to make all modules testable? In this case, the round down logic, and number formatting logic. They may be re-used by other places in the future, so better define it in a more standardized way. Plus, the mutation observer, which is definitely gonna be re-used.
There was a problem hiding this comment.
Ahh I see. Good question. There is not a general consensus on that question to be honest. Some say "There is a reason that a function is private/not exported", others say "it's better to have all code under test". Having everything under test can lead to having to rewrite the unit tests more often when you refactor, but for the state the codebase is in right now, I would lean to let's have everything testable. Because generally I would say the functions are doing quite a lot of things and once we have everything covered in tests I would love to break specific parts up in more atomic functions. Those could then be private/not exported/untested.
| expect(getNumberFormatter()).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| it("should return a correct formatter when using the URL locale", () => { |
There was a problem hiding this comment.
Extra info: I am planning to write the acceptance tests in this format as mentioned in the summary already, because it makes it easier to cover the codebase with tests. If I assume there is a bug in the code, I write the test so that they are green with the current code, but also add a section what the correct test code should be to trigger the buggy behaviour.
These can be addressed later on then, when code coverage is complete. By doing this approach we can minimize introducing new bugs to the codebase by refactoring.
| collectCoverage: false, | ||
| collectCoverageFrom: [ | ||
| "Extensions/combined/src/**/*.js", | ||
| "Extensions/combined/*.js", | ||
| ], |
There was a problem hiding this comment.
Disabled code coverage collection for now for performance reasons, but it can be enabled by changing collectCoverage to true
|
Amazing work, this was in my plans for a looong time. |
|
@Anarios (or anyone else who knows): I was gone for some time before opening this PR so I am not sure what the current approval/review workflow is. I see there are 6 reviews required. Do I have to actively request those, or what's the best process? |
Hey everyone,
I had a look at the codebase again and there are some opportunities to refactor thinks that were build back in the days, but before we do that we should have tests in place to make sure we don't introduce bugs while refactoring.
That's why I added jest for automated unit and integration testing to the project. I didn't write tests for everything yet, but started with the
utils.jssince they were easy to take apart.Two things I made sure to do:
The reason behind that is, that it is easier to strictly cover the codebase with tests first and have them as a contract for existing code and later go ahead and change the codebase with the tests as security net.
I used jest as the testing framework because it's in my opinion the most widely used and we can expand it to integration tests as well later on as well (which we should do to minimize manual testing needed when releasing changes in the future)
Let me know what you think of the changes, I'll try and keep my changes conflict free until it get's merged (or declined)
That's the current code coverage report: