Improve JS (Frontend) unit testing coverage

rugk commented 2 weeks ago

The problem

The code coverage of the JS tests is lacking…

Unit tests: This could have been detected by JS unit tests, but those are much weaker than our PHP ones and only cover about 62% of the logic, compared to 85% (most modules are above 95%, but the cloud storage backends are badly or not at all tested, dragging the score down). In particular, the problematic line is only covered one time and the bootstrap(3) specific event handler isn't covered at all.

That was written in an aftermath of a buggy release, we think we should probably improve the code here.

The solution

write tests, improve code coverage
- this can be done step by step, adding small tests etc.
Note big changes are likely easier when some refactoring like https://github.com/PrivateBin/PrivateBin/issues/365 is done

Alternatives

N/A e2e testing is related, but IMHO not strictly an alternative: https://github.com/PrivateBin/PrivateBin/issues/1335

Additional context

This issue has been discovered as a lessons learned from previous releases, which often had bugs being introduced. To have more confidence on code changes etc.

The important bit: Dear reader, if you feel like this is a thing you could contribute knowledge or actually doing it, feel free to comment here and help! This will greatly help the PrivateBin project. Also, as said above, you can start small and just contribute one or two tests and it will help the coverage to increase!

ankiiisharma commented 2 weeks ago

hey @rugk , can you please elaborate this issue more and can you please assign this to me?

rugk commented 2 weeks ago

What elaboration do you need?

Here there are more information on how to run the unit tests:

ankiiisharma commented 2 weeks ago

hey @rugk @elrido ,

These are the potential changes we can make to address the JavaScript test coverage issue :

Identify Untested Areas: Use coverage reports to pinpoint and target untested parts, like the bootstrap(3) specific event handler.
Add Unit Tests: Write new tests for uncovered functionalities, ensuring coverage of edge cases.
Refactor to ES6: Modernize JavaScript code to ES6+ standards for easier testing and maintenance.

Should I proceed with this? or do you guys have anything else in your mind.

elrido commented 2 weeks ago

If you can stomach it, personally I would prefer if you could focus on adding more test coverage. Would it be an option for you to do a first pass looking over for completely non-covered functions and of those the ones that either just read things from the DOM or that change the DOM? Those are relatively easy to test. Just create a minimal snippet of HTML that gets manipulated, i.e. by copying one from the source view of a live instance. Run the function, check for expected results.

There are areas that we can't test on a node environment, like the drag-n-drop APIs or those that autoselect text. Those are either browser-only APIs or simply not something one can easily detect from the DOM-tree properties alone.

To me, #198 seems more important then the mostly semantic changes of #365 - We have a history of spending a lot of time to refactor code without actually adding features. It's easy to get lost in a rabbit hole with these.

ankiiisharma commented 2 weeks ago

@elrido , got it so basically you need to add more test coverages. for example in AttachmentViewer.js we are handling attachments by setting, showing, hiding, and removing , previews and creating downloadable links. We can add more test coverages for Handling Special characters in filename, large file attachments, multiple attachments concurrently, etc.

right?

ankiiisharma commented 2 weeks ago

example :

let specialCharsClean = jsdom(),
          specialCharsData = "data:" + mimeType + ";base64," + btoa(rawdata),
          specialCharsResults = [];
        specialCharsFilename = `special_chars_${specialCharsFilename}!@#$%^&*()_+=-{}[];:'"\\|,.<>?`;
        $("body").html(
          '<div id="attachment" role="alert" class="hidden alert ' +
            'alert-info"><span class="glyphicon glyphicon-download-' +
            'alt" aria-hidden="true"></span> <a class="alert-link">' +
            'Download attachment</a></div><div id="attachmentPrevie' +
            'w" class="hidden"></div>'
        );
        $.PrivateBin.AttachmentViewer.init();
        $.PrivateBin.AttachmentViewer.setAttachment(
          specialCharsData,
          specialCharsFilename
        );
        const specialCharsAttachment =
          $.PrivateBin.AttachmentViewer.getAttachment();
        specialCharsResults.push(
          specialCharsAttachment[1] === specialCharsFilename
        );

elrido commented 2 weeks ago

The AttachmentViewer is already well covered, except for the parts like this one: AttachmentViewer

These are APIs (drag-n-drop, clip-board) that are not emulated by the jsDOM framework under node.

Also, we already generate random input using the jsVerify framework, so special chars should already be covered by the existing test (string covers any valid unicode sequence): https://github.com/PrivateBin/PrivateBin/blob/2324e83b84fb8f682d057dbe038f8b5849f8db61/js/test/AttachmentViewer.js#L8-L15

New tests should ideally get written to leverage the same technique and work with fuzzed input. This is explained in more detail in the development link rugk shared above and you can find the existing assets we created in the common.js: https://github.com/PrivateBin/PrivateBin/blob/2324e83b84fb8f682d057dbe038f8b5849f8db61/js/common.js#L85-L154

I think a good candidate to start at is TopNav, where there are no tests yet from rawText onwards (but skip updateExpiration and updateFormat - those are specific for the bootstrap(3) template and should get removed when we cleanup that template)

ankiiisharma commented 2 weeks ago

Got it!

ankiiisharma commented 2 weeks ago

Hello @elrido , I just raised PR #1338 . I added a small update for the rawText function. I made a few changes based on my understanding so far. Could you please check if everything is okay? regards.

elrido commented 2 weeks ago

Thank you, it checks out and did increase our code coverage. For that particular function there is not much more to test. Many of those TopNav functions simply hide or display sets of related navigation elements.

elrido commented 2 weeks ago

@ankiiisharma Weirdly, your new test did pass on the machine I tested it on, but failed when running the full test suite in github actions. Digging in, I realized that a) your test would target more the TopNav.hideAllButtons function, so I renamed it to that and b) TopNav.rawTextis a private function that you can't call directly. But you can trigger it through a click event on the button that the TopNav.initwill register it on:

https://github.com/PrivateBin/PrivateBin/blob/f9f8f187811613e85af0ce656386e9578d0563cd/js/test/TopNav.js#L684-L745

Finally, and that took me some trial and error to work out, this function interacts with the window.location URL and the window.history, so I needed to also reset the Helper and setup jsdom for URL handling. It's been a while since I've been writing these and my memory on dealing with these types of interactions has been getting a bit hazy.

This latter test could be improved by feeding PasteViewer.setText with random strings, but the validation would have to be more lenient, since rawText uses dompurify to sanitize malicious content before injecting it into the pre-tag. We'd essentially be testing dompurify, so I thought a validation that the DOM gets updated and the sample inserted into a pre-tag is a sufficient test for now.

ankiiisharma commented 2 weeks ago

Hey @elrido , sorry about that. Even i checked this function individually and it was running well. Let me check this.

elrido commented 2 weeks ago

I think they are fine for now and you can certainly move on and go for another one. I'll now again better understand what to look for when reviewing these. Just wanted to share with you my findings, so that you can benefit as well.

PrivateBin / PrivateBin