fix: Return inapplicable results (#473).

[robdodson]

No description provided.

[CLAassistant]

All committers have signed the CLA.

[robdodson]

@WilcoFiers I think this is ready for review if you have a moment.

[marcysutton]

Sweet, thanks for the contribution @robdodson! Do you mind adding a test for this? It looks like there's already a file: https://github.com/dequelabs/axe-core/blob/develop/test/core/utils/aggregateRule.js

[robdodson]

I'm not sure which solution y'all prefer. Because there are no nodes associated with an inapplicable test, it means the subResults array is empty.

And that empty array keeps getting passed around until eventually we reach aggregate().

It expects its second argument to not be an empty array. So when it finally calls sorting.pop() it's trying to pop an empty array, and ends up returning undefined.

I feel like it's almost easier to avoid going into aggregate() with an empty array of values but you tell me if you'd like something different. I think the nodes will always be an empty array by the very fact that they're inapplicable and didn't match anything.

[WilcoFiers]

I think we should rename some stuff here. I think aggregateRule should be called aggregateNodeResults and its paramater shouldn't be called subResults but nodeResults. That makes it a lot clearer why this fix works in the first place. Other then that, good fix, totally agree with how you did it.

[robdodson]

Thanks for taking a look @WilcoFiers :)

I've renamed the function and updated all of the tests that referenced it. Ready for review again when you have a moment.

[WilcoFiers]

Looks excellent to me. Thanks Rob! @marcysutton, anything to add?

[marcysutton]

Looks good to me! I was going to say we could add more details to the jsdoc block, but the utils methods aren't nearly as well commented as the other commons. We could tackle them in a separate PR.

[WilcoFiers]

Added it to 2x as well, so it'll be part of the 2.6 release. Thanks Rob!

[robdodson]

🎉 so glad it made the cut. Thanks for the quick reviews!

[dylanb]

@robdodson woot!!!