core(rel=noopener-audit): Only test http/https links

[sanjsanj]

Closes #3714

Only filter anchor links that start with an http protocol for the rel=noopener audit.

[sanjsanj]

Fixing the tests now...

[patrickhulce]

thanks for the quick patch @sanjsanj!! 👏

would you mind adding a few sample test cases to catch this failure?

the smoke tests file is over here

lighthouse/lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html

Lines 103 to 113 in f49ef06

	<template id="noopener-links-tmpl">
	<!-- FAIL - does not use rel="noopener" to open external link -->
	<a href="https://www.google.com/" target="_blank">external link</a>
	<!-- FAIL - does not use rel="noopener" and has no href attribute, giving an
	href value of '' when read, which will throw in a `new URL('')` constructor -->
	<a target="_blank">external link</a>
	<!-- PASS -->
	<a href="https://www.google.com/" target="_blank" rel="noopener nofollow">external link that uses rel noopener and another unrelated rel attribute</a>
	<!-- PASS -->
	<a href="./doesnotexist" target="_blank">internal link is ok</a>
	</template>

and unit tests over here

lighthouse/lighthouse-core/test/audits/dobetterweb/external-anchors-use-rel-noopener-test.js

Lines 43 to 68 in f49ef06

	it('handles links with no href attribute', () => {
	const auditResult = ExternalAnchorsAudit.audit({
	AnchorsWithNoRelNoopener: [
	{href: ''},
	{href: 'http://'},
	{href: 'http:'},
	],
	URL: {finalUrl: URL},
	});
	assert.equal(auditResult.rawValue, false);
	assert.equal(auditResult.details.items.length, 3);
	assert.equal(auditResult.details.items.length, 3);
	assert.ok(auditResult.debugString, 'includes debugString');
	});
	it('does not fail for links with javascript in href attribute', () => {
	const auditResult = ExternalAnchorsAudit.audit({
	AnchorsWithNoRelNoopener: [
	{href: 'javascript:void(0)'},
	{href: 'JAVASCRIPT:void(0)'},
	],
	URL: {finalUrl: URL},
	});
	assert.equal(auditResult.rawValue, true);
	assert.equal(auditResult.details.items.length, 0);
	});

can throw in a check for handling href === null in unit tests and the mail links in both cases

[patrickhulce]

probably need a anchor.href && here to be safe

[sanjsanj]

@patrickhulce I meant to ask about that, it was previously checking for !anchor.href, and the unit tests seem to suggest that's what it's wanting to do, so I've put that back in.

lighthouse/lighthouse-core/test/audits/dobetterweb/external-anchors-use-rel-noopener-test.js

Lines 43 to 49 in f49ef06

	it('handles links with no href attribute', () => {
	const auditResult = ExternalAnchorsAudit.audit({
	AnchorsWithNoRelNoopener: [
	{href: ''},
	{href: 'http://'},
	{href: 'http:'},
	],

[sanjsanj]

Unless this unit test is wrong and this audit should ignore anchors without external links, and I presume you need an href to be an external link.

[patrickhulce]

oh whoops, yeah that's right you can ignore my comment then :)

when glancing I assumed it was there as a null guard for the next case, but I remember now it was intentional

[sanjsanj]

@patrickhulce Thanks for the fast feedback! Yup sure, leave it with me I'll get that done asap.

[sanjsanj]

Ok I've added a handful more tests. While I was at it I couldn't resist tidying them up a bit;

• Group passes and fails together to make it a bit more organised
• Add comments where there weren't any
• Separate the href=null and href="http*" unit test out into two separate tests since they're testing different functionality (imo)
• Unify the grammar a bit

[sanjsanj]

Sorry again, fixing the tests... :)

[sanjsanj]

Sorry, forgot to update after fixing the failing tests but the PR is ready for more feedback when you have time, TY!

[patrickhulce]

thanks for jumping in here on this @sanjsanj! congrats on your first contribution!! 🎉 🥇

[sanjsanj]

Sweeeet, cheers @patrickhulce !