marcelduran / yslow

YSlow analyzes web pages and suggests ways to improve their performance based on a set of rules for high performance web pages.
yslow.org
BSD 3-Clause "New" or "Revised" License
2.2k stars 383 forks source link

TAP results have parse errors in Jenkins when CDN is specified #103

Open gurdenbatra opened 10 years ago

gurdenbatra commented 10 years ago

When I run this on Jenkins-

phantomjs redfin.qa/redfin.qa.phantomScripts/yslow.js -i grade --cdns "img.cdn-redfin.com" -t '{"overall": "A", "ycdn": "F", "yexpires": "C"}' -f tap http://www.redfin.com/ > yslow.tap

I get- TAP Extended Test Results: http://cl.ly/image/172Y2B2w3f3J TAP Test Results: http://cl.ly/image/3z3x0t1y0612

When I run the command without specifying the CDN-

phantomjs redfin.qa/redfin.qa.phantomScripts/yslow.js -i grade -t '{"overall": "A", "ycdn": "F", "yexpires": "C"}' -f tap http://www.redfin.com/ > yslow.tap

I get results- TAP Extended Test Results: http://cl.ly/image/0X1t1j0A2h1l TAP Test Results: http://cl.ly/image/3w3A3B301c0x

remmons commented 10 years ago

Have you found a work around for this?

gurdenbatra commented 10 years ago

Nope, I just lowered the grade thresholds :(

kinow commented 10 years ago

Hello there. If there's anything that can be done in TAP plug-in just let me know :o) Being busy with $work projects in the past months, but I have some tasks in Jenkins plug-ins too this week. Maybe I can take a look in TAP plug-ins issues/pull requests.

Bruno

marcelduran commented 10 years ago

Comparing both TAP files side-by-side the only thing that might be causing parse error is the colon right after preferences in "Using these CDN hostnames from your preferences: img.cdn-redfin.com". Not sure if Jenkins TAP parser is attempting to make preferences: ... an actual key-value entry. Is there a valid way to either escape such colon or surround the whole message value with (double)quotes?

kinow commented 10 years ago

Could be @marcelduran . Can you make a gist with the TAP stream, please? I'll debug it in tap4j to check what's going on.

marcelduran commented 10 years ago

@kinow You got it: https://gist.github.com/marcelduran/9157cd87f91732c907fe

kinow commented 10 years ago

Hi @marcelduran! Thanks!

The following YAML is not valid:

message: There are 2 static components that are not on CDN. <p>Using these CDN hostnames from your preferences: img.cdn-redfin.com</p>
offenders:
  - "www.redfin.com: 1 component, 313.1K (313.1K GZip)"
  - "pixel.quantserve.com: 1 component, 0.03K"

InstantYAML confirms that it is not valid due to that colon you mentioned earlier. The easiest way to fix it is by surrounding your message text with ", as below:

message: "There are 2 static components that are not on CDN. <p>Using these CDN hostnames from your preferences: img.cdn-redfin.com</p>"
offenders:
  - "www.redfin.com: 1 component, 313.1K (313.1K GZip)"
  - "pixel.quantserve.com: 1 component, 0.03K"

I tried that in tap-plugin, using the underlying tap4j code, and all looked good.

Does that sound like a doable solution?

Cheers

marcelduran commented 10 years ago

Awesome! It might work for that case but my concern is for cases where there are HTML attributes like:

message: "There are 2 static components that are not on CDN. <p>Using these CDN hostnames from your preferences: img.cdn-redfin.com</p><a href="some_link">More info</a>" offenders:

Can I escape inner quotes? How?

kinow commented 10 years ago

Hmm, you can try replacing

<p>Using these CDN hostnames...

By

&lt;p&gt;Using these CDN hostnames...

I think someone reported an issue for the TAP plug-in a year ago on that, but I can't recall what was the solution found at that time.

Would that work?

marcelduran commented 10 years ago

Well, back in the day, the offender list renderer was first designed for FF only and reused across several other platforms until landed in PhantomJS. It certainly needs some refactoring to be platform agnostic, therefore outputting clean messages. I'll take a look on it and will try to remove any html from output as a quick workaround. Thanks!

nak1 commented 8 years ago

Has there been a fix to this issue? I'm experiencing now Jenkins TAP plugin v 2.0.1.