Improve JS (Frontend) unit testing coverage

[rugk]

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 Modernize JS (ES6 and up) #365 is done

Alternatives

N/A
e2e testing is related, but IMHO not strictly an alternative: #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]

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

[rugk]

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

[ankiiisharma]

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]

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]

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

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]

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

PrivateBin/js/test/AttachmentViewer.js

Lines 8 to 15 in 2324e83

	jsc.property(
	'displays & hides data as requested',
	common.jscMimeTypes(),
	'string',
	'string',
	'string',
	'string',
	function (mimeType, rawdata, filename, prefix, postfix) {

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:

PrivateBin/js/common.js

Lines 85 to 154 in 2324e83

	// provides random lowercase characters from a to z
	exports.jscA2zString = function() {
	return jsc.elements(a2zString);
	};
	// provides random lowercase alpha numeric characters (a to z and 0 to 9)
	exports.jscAlnumString = function() {
	return jsc.elements(alnumString);
	};
	//provides random characters allowed in hexadecimal notation
	exports.jscHexString = function() {
	return jsc.elements(hexString);
	};
	// provides random characters allowed in GET queries
	exports.jscQueryString = function() {
	return jsc.elements(queryString);
	};
	// provides random characters allowed in hash queries
	exports.jscHashString = function() {
	return jsc.elements(hashString);
	};
	// provides random characters allowed in base64 encoded strings
	exports.jscBase64String = function() {
	return jsc.elements(base64String);
	};
	// provides a random URL schema supported by the whatwg-url library
	exports.jscSchemas = function(withFtp = true) {
	return jsc.elements(withFtp ? schemas : schemas.slice(1));
	};
	// provides a random supported language string
	exports.jscSupportedLanguages = function() {
	return jsc.elements(supportedLanguages);
	};
	// provides a random mime type
	exports.jscMimeTypes = function() {
	return jsc.elements(mimeTypes);
	};
	// provides a random PrivateBin paste formatter
	exports.jscFormats = function() {
	return jsc.elements(formats);
	};
	// provides random URLs
	exports.jscUrl = function(withFragment = true, withQuery = true) {
	let url = {
	schema: exports.jscSchemas(),
	address: jsc.nearray(exports.jscA2zString()),
	};
	if (withFragment) {
	url.fragment = jsc.string;
	}
	if(withQuery) {
	url.query = jsc.array(exports.jscQueryString());
	}
	return jsc.record(url);
	};
	exports.urlToString = function (url) {
	return url.schema + '://' + url.address.join('') + '/' + (url.query ? '?' +
	encodeURI(url.query.join('').replace(/^&+|&+$/gm,'')) : '') +
	(url.fragment ? '#' + encodeURI(url.fragment) : '');
	};

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]

Got it!

[ankiiisharma]

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]

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]

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

PrivateBin/js/test/TopNav.js

Lines 684 to 745 in f9f8f18

	describe('hideAllButtons', function () {
	before(function () {
	cleanup();
	});
	it(
	'hides all buttons correctly',
	function () {
	// Insert any setup code needed for the hideAllButtons function
	// Example: Initialize the DOM elements required for testing
	$('body').html(
	'<nav class="navbar navbar-inverse navbar-static-top">' +
	'<div id="navbar" class="navbar-collapse collapse"><ul ' +
	'class="nav navbar-nav"><li><button id="newbutton" ' +
	'type="button" class="hidden btn btn-warning navbar-btn">' +
	'<span class="glyphicon glyphicon-file" aria-hidden="true">' +
	'</span> New</button><button id="clonebutton" type="button"' +
	' class="hidden btn btn-warning navbar-btn">' +
	'<span class="glyphicon glyphicon-duplicate" ' +
	'aria-hidden="true"></span> Clone</button><button ' +
	'id="rawtextbutton" type="button" class="hidden btn ' +
	'btn-warning navbar-btn"><span class="glyphicon ' +
	'glyphicon-text-background" aria-hidden="true"></span> ' +
	'Raw text</button><button id="qrcodelink" type="button" ' +
	'data-toggle="modal" data-target="#qrcodemodal" ' +
	'class="hidden btn btn-warning navbar-btn"><span ' +
	'class="glyphicon glyphicon-qrcode" aria-hidden="true">' +
	'</span> QR code</button></li></ul></div></nav>'
	);
	$.PrivateBin.TopNav.init();
	$.PrivateBin.TopNav.hideAllButtons();
	assert.ok($('#newbutton').hasClass('hidden'));
	assert.ok($('#clonebutton').hasClass('hidden'));
	assert.ok($('#rawtextbutton').hasClass('hidden'));
	assert.ok($('#qrcodelink').hasClass('hidden'));
	cleanup();
	}
	);
	});
	describe('rawText', function () {
	before(function () {
	cleanup();
	});
	it(
	'displays raw text view correctly',
	function () {
	const clean = jsdom('', {url: 'https://privatebin.net/?0123456789abcdef#0'});
	global.URL = require('jsdom-url').URL;
	$('body').html('<button id="rawtextbutton"></button>');
	const sample = 'example';
	$.PrivateBin.PasteViewer.setText(sample);
	$.PrivateBin.Helper.reset();
	$.PrivateBin.TopNav.init();
	$('#rawtextbutton').click();
	assert.equal($('pre').text(), sample);
	clean();
	}
	);
	});

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]

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

[elrido]

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.