Fixes #933, manifest display debugString. Add test coverage. #950
Conversation
ebidel
changed the title
| }; | ||
|
|
||
| const display = artifacts.Manifest.value.display; | ||
| display.raw = display.value = ''; |
| @@ -53,9 +53,14 @@ class ManifestDisplay extends Audit { | |||
| return ManifestDisplay.generateAuditResult({ | |||
| rawValue: hasRecommendedValue, | |||
| displayValue, | |||
| debugString: 'Manifest display property should be set.' | |||
| debugString: this.createDebugString(hasRecommendedValue) | |||
There was a problem hiding this comment.
If there's no value, it might be better not to add the debugString property in the first place. It's presence, even if blank, indicates some kind of error.
There was a problem hiding this comment.
@ebidel @brendankenny I was following the pattern I found here, and saw that it was a fairly consistent pattern throughout the audits. ( I could be mistaken, though!)
So, undefined is > empty string ?
A couple of ex:
aria-allowed-attr
color-contrast
There was a problem hiding this comment.
So,
undefinedis > empty string ?
I'd say yes. At some point we should change all audits to standardize on this
There was a problem hiding this comment.
What @brendankenny said :) It's our bad that things are inconsistent atm.
There was a problem hiding this comment.
I have somewhat mixed feelings about debugString: undefined but can live with what you have.
I was thinking we'd remove createDebugString and just roll with:
const auditResult = ManifestDisplay.generateAuditResult({
rawValue: hasRecommendedValue,
displayValue
});
if (!hasRecommendedValue) {
auditResult.debugString = 'Manifest display property should be set.';
}
return auditResult;
That way, the debugString property only gets added when there's an error.
debugString: hasRecValue ? undefined : debugString; works but it stills add the property to the artifacts object. Oher parts of the codebase may make a check like 'debugString' in auditResult, which would return true even though there was no error.
There was a problem hiding this comment.
Ah, I was a little confused between what @brendankenny mentioned below.
I think it would be better to just inline a test for debugString to exist or not in the above tests instead of adding new tests just for debugString
Now, the property is absent from the object if valid, i.e. what you pasted above.
| @@ -66,4 +66,28 @@ describe('Mobile-friendly: display audit', () => { | |||
| it('succeeds when a complete manifest contains a display property', () => { | |||
| return assert.equal(Audit.audit({Manifest: exampleManifest}).rawValue, true); | |||
| }); | |||
|
|
|||
| it('has no debugString when successful', () => { | |||
There was a problem hiding this comment.
I think it would be better to just inline a test for debugString to exist or not in the above tests instead of adding new tests just for debugString
|
|
||
| static createDebugString(hasRecValue) { | ||
| const debugString = 'Manifest display property should be set.'; | ||
| return hasRecValue ? '' : debugString; |
There was a problem hiding this comment.
definitely agree that debugString should be undefined if no error. Doing return hasRecValue ? undefined : debugString; here would be sufficient
|
@ebidel @brendankenny |
| return auditResult; | ||
| } | ||
|
|
||
| static createDebugString(hasRecValue) { |
|
@brendankenny Oh, bummer. Looks like I should have gotten to it sooner. |
|
no problem, sorry about that, just really needed the change :) |
|
arg, I tried to make sure your commit was preserved to give you credit for the contribution but somewhere between cherry picking and squashing it was lost. Sorry about that :( You still have credit in our hearts |
|
Not urgent immediacy, but I was confident that github would handle the merge correctly and it would be a safe maneuver, and it was a nice change to have in the new release. However, I wouldn't have pushed forward on merging if I had known the commit author would be changed. It's extremely important to us to recognize everyone who contributes to the project, so for whatever it's worth, really sorry that happened :( |
fixes #933