search

catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.93k stars 563 forks source link

Proposal: Vulcanized Trace Viewer instrumentation test #1715

Open petrcermak opened 9 years ago

petrcermak commented 9 years ago

It turns out that some bugs only occur once Trace Viewer is vulcanized (e.g. #1714). Therefore, I think that it would be a good idea to have some instrumentation tests for vulcanized Trace Viewer as well. The bare minimum would be a test that checks that the vulcanized HTML file loads in a browser without any exceptions.

@natduca, @dj2: WDYT?

natduca commented 9 years ago

Hmmm... thats not a bad idea. @anniesullie @qyearsley for thoughts.

In the #1714 case, the error makes me think that the vulcanized result we're producing isn't itself in strict mode? Is that intentional or accidnetal I wonder?

anniesullie commented 9 years ago

Yeah, we should definitely do this. Seems like it should be possible to do with modifications to run_dev_server_tests.py, just not sure how to do it in a general way instead of a hacked on way.

natduca commented 9 years ago

I like the idea of starting simple and just opening the vulcanized file and seeing if we have window errors.

Running the tests on the vulcanized data I think might be tough.

petrcermak commented 9 years ago

I was also quite surprised to learn that the vulcanized file is not in strict mode (or at least not in Chrome's "opinion").

I had a quick look at trace_viewer_full.html and the problem is that the vulcanized file contains a single tags.

anniesullie commented 9 years ago

@nedn since he's done some tests of vulcanization + performance

nedn commented 9 years ago

Yeah, I also fixed a few bug when sending out https://codereview.chromium.org/1417883005/ for review. We should add: "trace-viewer test with vulcanize + vinn test with vulcanize"

nedn commented 9 years ago

For the issue of 'use strict', how about we tell py_vulcanize to add 'use strict' as the first statement of < script> block? I would prefer we be able to keep the single < script> tag since that would enable further minification down the road.

petrcermak commented 9 years ago

It's probably worth trying, but it might break and/or slightly change the behavior of some third-party dependencies.

nedn commented 9 years ago

As a fallback, if the third-party dep once are problem with 'use strict', I think we can add some special bit so that py_vulcanize be smarter about adding them. <script src='third_party.js' inline="false">

natduca commented 9 years ago

Lemme back up and deal witih fundamentals... we have some scripts that aren't strict mode. So we can't just force them to strict mode. That would be a breaking change, too.

Fortunately, strictness is a function level thing. So, one thing you can do is put each file in (function() { ... })() blocks. That'd probably create a lot of useless function scoping though, so we may want to group files together when they're all strict mode. Shrug.