Improve JS (Frontend) unit testing coverage about privatebin HOT 13 OPEN

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

from privatebin.

What elaboration do you need?

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

• https://github.com/PrivateBin/PrivateBin/wiki/Development#unit-tests
• https://github.com/PrivateBin/PrivateBin/blob/master/doc/Running%20Unit%20Tests.md

from privatebin.

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.

from privatebin.

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.

from privatebin.

@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?

from privatebin.

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
        );

from privatebin.

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):

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:

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)

from privatebin.

Got it!

from privatebin.

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.

from privatebin.

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.

from privatebin.

@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:

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.

from privatebin.

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

from privatebin.

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.

from privatebin.