core(driver): warn about remaining inflight requests urls

[doteric]

Summary

Add extra warning to show inflight requests if the load timeout is reached.

The reason of this small and simple change is to give the user a better logging experience to know what's wrong.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[connorjclark]

inflightRequestUrls

[doteric]

Updated

[connorjclark]

Suggested change

	"Remaining inflight requests URLs",
	"Remaining inflight requests",

[adamraine]

This line could be really long if there are multiple requests, maybe we should have each request on its own line.

[doteric]

Hey @connorjclark and @adamraine
Thanks for the feedback 💪

From my point of view as long as it's an array that common console/terminal formatters can handle then it should be fine. But maybe you'd like me to add something along the lines of show first 5 requests and after that ...and X more requests for example? That way it should be pretty neat in case of a lot of remaining inflight requests🤔

[connorjclark]

The debug module is going to format the object given using the inspect interface (same as if you console logged it). For arrays, that elides the elements.

My preference is to join the urls using , , and keeping it to a single warn message.

[doteric]

@connorjclark Thanks for checking this 💪

My preference is to join the urls using , , and keeping it to a single warn message.

Do you mean like you posted on your screenshot or inflightRequestsUrl.join(',')? I would prefer the first option so the way you posted it on your screenshot 🤔

[connorjclark]

inflightRequestsUrl.join(', '). There shouldn't be on the order of hundreds (or even dozens...) of in flight requests, so I don't think we'd ever even see this log message being elided.

[doteric]

If you'd prefer that then sure, but what do you think about just keeping inflightRequestsUrl and let the terminals (or wherever the logs are) to handle the formatting? Seems more safe than joining with , 🤔
But if you'd heavily prefer inflightRequestsUrl.join(', ') then just let me know and I'll change it to that and we can proceed 😁
Thank you for the review btw 💪

[doteric]

bump
@adamraine & @connorjclark let me know please :D

[doteric]

What's the recommended approach to mocking log to test if it has been called correctly? I found importMock, but it doesn't seem to support mocking external modules.

[brendankenny]

log.events will emit events when something is logged (edit, well, log.warned, at least), so you can collect those events and assert those without mocking. One example:

lighthouse/core/test/config/filters-test.js

Lines 471 to 482 in 2b8caf2

	/** @type {Array<unknown>} */
	const warnings = [];
	/** @param {unknown} evt */
	const saveWarning = evt => warnings.push(evt);
	log.events.on('warning', saveWarning);
	const filtered = filters.filterConfigByExplicitFilters(resolvedConfig, {
	onlyAudits: null,
	onlyCategories: ['timespan', 'thisIsNotACategory'],
	skipAudits: null,
	});
	log.events.off('warning', saveWarning);

[doteric]

Let me know if the test is fine or you would like something more and I'll add it :D
codecov seems to be complaining for some reason though as if it didn't catch the test :/