Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1662833005:
Fixes Caterpillar crashing while generating report. (Closed)
|
Created:
4 years, 10 months ago by Matthew Alger Modified:
4 years, 10 months ago Reviewers:
Matt Giuca CC:
raymes Base URL:
https://github.com/chromium/caterpillar.git@master Target Ref:
refs/heads/master Project:
caterpillar Visibility:
Public. |
DescriptionFixes Caterpillar crashing while generating report.
Caterpillar currently crashes while generating a report. This
is due to API changes between writing and merging the report
linking branch. This CL updates the report code to account for
the new API and new variable names.
Tests to catch the problem if it arises again in future are
also added in this CL.
Resolves #42. https://github.com/chromium/caterpillar/issues/42
R=mgiuca@chromium.org
Committed: dc0b8af616f016fe9fa3e9eeeea4b12b709d69c3
Patch Set 1 #
Total comments: 2
Patch Set 2 : Response to CR #
Total comments: 6
Patch Set 3 : Response to CR #
Total comments: 1
Patch Set 4 : Changed setUp to only run once for all end-to-end tests. #
Total comments: 2
Patch Set 5 : Reverted most changes except critical bugfixes and associated tests. #Patch Set 6 : Skipped broken test. #
Messages
Total messages: 21 (8 generated)
Description was changed from ========== Updates end-to-end tests with report. Fixes report merging bugs. ========== to ========== Updates end-to-end tests with report. Fixes report merging bugs. This CL updates the report linking based on recent changes to master, and fixes a bug that was missed due to the lack of end-to-end tests for report linking. ==========
alger@google.com changed reviewers: + mgiuca@chromium.org
Missing files diff.
diff --git "a/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/bower_components/Inconsolata/This folder
intentionally left blank" "b/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/bower_components/Inconsolata/This folder
intentionally left blank"
new file mode 100644
index 0000000..e69de29
diff --git "a/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/bower_components/code-prettify/This folder
intentionally left blank" "b/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/bower_components/code-prettify/This folder
intentionally left blank"
new file mode 100644
index 0000000..e69de29
diff --git "a/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/bower_components/lato/This folder
intentionally left blank" "b/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/bower_components/lato/This folder
intentionally left blank"
new file mode 100644
index 0000000..e69de29
diff --git "a/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/report.css"
"b/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/report.css"
new file mode 100644
index 0000000..2a924f1
--- /dev/null
+++ "b/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/report.css"
@@ -0,0 +1 @@
+/* This file intentionally left blank. */
diff --git "a/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/report.html"
"b/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/report.html"
new file mode 100644
index 0000000..7b278fc
--- /dev/null
+++ "b/tests/test_app_tts_output/report
\342\234\223\342\234\223\342\234\223/report.html"
@@ -0,0 +1,190 @@
+
+<!DOCTYPE html>
+<html lang="en">
+ <head>
+ <meta charset="utf-8">
+ <title>Caterpillar Conversion Report: Text-to-Speech Chrome App</title>
+ <link rel="stylesheet" href="report.css">
+ <body>
+ <div id="report">
+ <h1>
+ Caterpillar Conversion Report:
+ <span class="name">Text-to-Speech Chrome App</span>
+ </h1>
+
+<section id="summary">
+ <h2>Summary</h2>
+ <span class="name">Text-to-Speech Chrome App</span> was
+ <span class="conversion-status total">
+
+ successfully
+
+ converted, with warnings.
+ </span>
+
+ <ul>
+
+ <li>
+ <span class="ca-feature none">
+ chrome.app.runtime
+ </span>
+ was not polyfilled.
+ </li>
+
+ <li>
+ <span class="ca-feature none">
+ chrome.app.window
+ </span>
+ was not polyfilled.
+ </li>
+
+ <li>
+ <span class="ca-feature total">
+ chrome.tts
+ </span>
+ was polyfilled.
+ </li>
+
+ </ul>
+</section>
+
+
+<section id="general-warnings">
+ <h2>General Warnings</h2>
+ <ul>
+
+ <li>Could not polyfill Chrome APIs: app.runtime, app.window</li>
+
+ <li>Manifest member `permissions` will not be converted.</li>
+
+ </ul>
+</section>
+
+
+
+<section id="polyfilled">
+ <h2>Polyfilled Chrome Apps APIs</h2>
+ <p>
+ Some Chrome Apps APIs have been polyfilled. Calls to these APIs will
+ generally still work, though there may be missing functionality. You should
+ check that the functionality you rely on still works.
+ </p>
+
+
+ <section class="polyfill">
+ <h3 class="ca-feature total">
+ chrome.tts
+ </h3>
+ <p>
+ <span class="ca-feature total">
+ chrome.tts
+ </span> was
+
+ polyfilled.
+ </p>
+
+ <p>
+ <span class="name">Text-to-Speech Chrome App</span> seems to make
+ use of features with missing or altered functionality in the
polyfill:
+ </p>
+ <ul>
+
+ <li><span class="ca-feature total">chrome.tts.speak</span>: No
longer supports extension ID.</li>
+
+ <li><span class="ca-feature total">chrome.tts.speak</span>: No
longer supports gender.</li>
+
+ <li><span class="ca-feature total">chrome.tts.speak</span>: No
longer supports required event types.</li>
+
+ </ul>
+
+
+ <p>
+ <span class="name">Text-to-Speech Chrome App</span> uses
+ <span class="ca-feature total">
+ tts
+ </span>
+ in the following places:
+ </p>
+
+ <p class="code-location path">πΌπ¬π»π²πΉπ½πΌ/ttstest.js:5</p>
+<pre>
+<code class="prettyprint lang-js linenums:3">while (true)
+// TODO(Caterpillar): Check usage of tts.speak.
+ <span class="ca-feature total">chrome.tts.speak</span>('π’ chrome apps! π’');
+// end chrome app
+</code>
+</pre>
+
+ </section>
+
+</section>
+
+
+
+<section id="not-polyfilled">
+ <h2>Missing Chrome Apps APIs</h2>
+ <p>
+ Some Chrome Apps APIs have not been polyfilled, and your application may
+ rely on them. Places they are used in your application are shown below.
+ </p>
+
+
+ <section class="missing-api">
+ <h3 class="ca-feature none">
+ chrome.app.runtime
+ </h3>
+ <p>
+ <span class="name">Text-to-Speech Chrome App</span> uses
+ <span class="ca-feature none">
+ app.runtime
+ </span>
+ in the following places:
+ </p>
+
+ <p class="code-location path">πΌπ¬π»π²πΉπ½πΌ/main.js:4</p>
+<pre>
+<code class="prettyprint lang-js linenums:2">// make a chrome app window
+// TODO(Caterpillar): Check usage of app.runtime.onLaunched.addListener.
+<span class="ca-feature
none">chrome.app.runtime.onLaunched.addListener</span>(function() {
+// TODO(Caterpillar): Check usage of app.window.create.
+ chrome.app.window.create('ttstest.html', {
+</code>
+</pre>
+
+ </section>
+
+ <section class="missing-api">
+ <h3 class="ca-feature none">
+ chrome.app.window
+ </h3>
+ <p>
+ <span class="name">Text-to-Speech Chrome App</span> uses
+ <span class="ca-feature none">
+ app.window
+ </span>
+ in the following places:
+ </p>
+
+ <p class="code-location path">πΌπ¬π»π²πΉπ½πΌ/main.js:6</p>
+<pre>
+<code class="prettyprint lang-js
linenums:4">chrome.app.runtime.onLaunched.addListener(function() {
+// TODO(Caterpillar): Check usage of app.window.create.
+ <span class="ca-feature none">chrome.app.window.create</span>('ttstest.html',
{
+ id: 'id'
+ });
+</code>
+</pre>
+
+ </section>
+
+</section>
+
+ </div>
+ <footer>
+ Generated by
+ <a href="https://github.com/chromium/caterpillar">Caterpillar</a>.
+ </footer>
+ <script src="bower_components/code-prettify/src/run_prettify.js">
+ </script>
+ </body>
+</html>
\ No newline at end of file
There was a problem uploading templates.py. Any idea why? I don't see how this updates end-to-end tests. There is no change to end_to_end_test.py other than trivial bower_components changes. Or is it the addition of new files in the expected output directory? (I don't see how the tests were not exercising that crash you fixed but now they are.) https://codereview.chromium.org/1662833005/diff/1/src/end_to_end_test.py File src/end_to_end_test.py (right): https://codereview.chromium.org/1662833005/diff/1/src/end_to_end_test.py#newc... src/end_to_end_test.py:83: for dirname, _, filenames in os.walk(TTS_REFERENCE_PATH): This has almost the same thing four times. I think it could be abstracted. Why might bower_components be in a directory and not match?
On 2016/02/04 05:10:31, Matt Giuca wrote: > There was a problem uploading templates.py. Any idea why? Looks fine to me, so maybe Rietveld sees Unicode characters and terminates immediately? > I don't see how this updates end-to-end tests. There is no change to > end_to_end_test.py other than trivial bower_components changes. Or is it the > addition of new files in the expected output directory? (I don't see how the > tests were not exercising that crash you fixed but now they are.) Addition of new files, so the generated report matches what we expect.
https://codereview.chromium.org/1662833005/diff/1/src/end_to_end_test.py File src/end_to_end_test.py (right): https://codereview.chromium.org/1662833005/diff/1/src/end_to_end_test.py#newc... src/end_to_end_test.py:83: for dirname, _, filenames in os.walk(TTS_REFERENCE_PATH): On 2016/02/04 05:10:31, Matt Giuca wrote: > This has almost the same thing four times. I think it could be abstracted. > > Why might bower_components be in a directory and not match? Done. We ignore bower_components because it is where we store report dependencies, and I don't want to check those.
Patchset #2 (id:20001) has been deleted
I am wondering if you a) fix the test so it raises an exception if the subprocess fails, and b) fix the bug (but do not change the directory comparison code or add the new test files), whether directory comparison would fail. Please test that so we make sure the test is properly testing comparison. https://codereview.chromium.org/1662833005/diff/40001/src/end_to_end_test.py File src/end_to_end_test.py (right): https://codereview.chromium.org/1662833005/diff/40001/src/end_to_end_test.py#... src/end_to_end_test.py:86: ignore_folders: List of folder names to ignore. directory / folder are interchangeable, so don't mix them. (Consistently use "directory".) Also make it a set. https://codereview.chromium.org/1662833005/diff/40001/src/end_to_end_test.py#... src/end_to_end_test.py:98: if folder in dirname: This is not precise enough. If ignore_folders = ['bower_components'], this will match any path with "bower_components" anywhere in the string, including if it is part of a directory name. Is |ignore_folders| supposed to be any folder called that, or only folders whose full path (relative to |directory|) matches? Have a look at os.walk with topdown=True. The documentation there explicitly mentions pruning the search. If |ignore_folders| is supposed to be any directory name, you can just delete any item in |dirnames| (in the result from walk) that is in ignore_folders, and you're done. https://codereview.chromium.org/1662833005/diff/40001/src/end_to_end_test.py#... src/end_to_end_test.py:116: ['bower_components']) {'bower_components'}
On 2016/02/04 06:36:34, Matt Giuca wrote: > I am wondering if you a) fix the test so it raises an exception if the > subprocess fails, and b) fix the bug (but do not change the directory comparison > code or add the new test files), whether directory comparison would fail. Please > test that so we make sure the test is properly testing comparison. Yup, directory comparison fails without the new files.
https://codereview.chromium.org/1662833005/diff/40001/src/end_to_end_test.py File src/end_to_end_test.py (right): https://codereview.chromium.org/1662833005/diff/40001/src/end_to_end_test.py#... src/end_to_end_test.py:86: ignore_folders: List of folder names to ignore. On 2016/02/04 06:36:34, Matt Giuca wrote: > directory / folder are interchangeable, so don't mix them. (Consistently use > "directory".) > > Also make it a set. Done. https://codereview.chromium.org/1662833005/diff/40001/src/end_to_end_test.py#... src/end_to_end_test.py:98: if folder in dirname: On 2016/02/04 06:36:34, Matt Giuca wrote: > This is not precise enough. If ignore_folders = ['bower_components'], this will > match any path with "bower_components" anywhere in the string, including if it > is part of a directory name. > > Is |ignore_folders| supposed to be any folder called that, or only folders whose > full path (relative to |directory|) matches? > > Have a look at os.walk with topdown=True. The documentation there explicitly > mentions pruning the search. If |ignore_folders| is supposed to be any directory > name, you can just delete any item in |dirnames| (in the result from walk) that > is in ignore_folders, and you're done. Oh, that's much cleaner. Done. https://codereview.chromium.org/1662833005/diff/40001/src/end_to_end_test.py#... src/end_to_end_test.py:116: ['bower_components']) On 2016/02/04 06:36:34, Matt Giuca wrote: > {'bower_components'} Done.
Since report generation is quite slow (as bower is called to install Lato/Inconsolata/Code Prettify dependencies) I've changed the setUp method to be a class method which runs once to set up all end-to-end tests. This makes testing ~3x faster.
OK as I've said in the comments, I think this CL needs to be broken up. This CL's description should first and foremost explain that it takes Caterpillar from always crashing to not crashing (the tests are ancillary). You can refactor / speed up the tests in another CL. https://codereview.chromium.org/1662833005/diff/60001/src/end_to_end_test.py File src/end_to_end_test.py (right): https://codereview.chromium.org/1662833005/diff/60001/src/end_to_end_test.py#... src/end_to_end_test.py:97: basename = os.path.basename(os.path.normpath(dirname)) This is descending into the ignored directory, and then deleting all of its subdirectories. Instead, you should just delete the ignored directory when it shows up in its parent (then you can just directory compare directory names without having to mess around with normpath and basename). # Do not descend into ignored directories. for i in range(len(dirnames) - 1, -1, -1): if dirnames[i] in ignore_directories: del dirnames[i] I think this is the intended usage of os.walk(topdown=true). https://codereview.chromium.org/1662833005/diff/80001/src/end_to_end_test.py File src/end_to_end_test.py (right): https://codereview.chromium.org/1662833005/diff/80001/src/end_to_end_test.py#... src/end_to_end_test.py:43: class TestEndToEndConvert(unittest.TestCase): Why no longer using TestCaseWithTempDir? https://codereview.chromium.org/1662833005/diff/80001/src/end_to_end_test.py#... src/end_to_end_test.py:79: output = subprocess.check_output( It's really quite strange that you run the entire test in setUp (now setUpClass), and just do checking in the individual test methods. But for performance reasons, I suppose this makes sense to do it. However, this shouldn't be done in this CL. This CL is fixing a major bug --- Caterpillar is broken and the tests aren't testing it. So you shouldn't be refactoring or fixing test performance here. Please do this later. Come to think of it, you should probably save the get_relative_filepaths refactor for that cleanup CL as well.
On 2016/02/05 00:23:49, Matt Giuca wrote: > OK as I've said in the comments, I think this CL needs to be broken up. This > CL's description should first and foremost explain that it takes Caterpillar > from always crashing to not crashing (the tests are ancillary). > > You can refactor / speed up the tests in another CL. > > https://codereview.chromium.org/1662833005/diff/60001/src/end_to_end_test.py > File src/end_to_end_test.py (right): > > https://codereview.chromium.org/1662833005/diff/60001/src/end_to_end_test.py#... > src/end_to_end_test.py:97: basename = > os.path.basename(os.path.normpath(dirname)) > This is descending into the ignored directory, and then deleting all of its > subdirectories. > > Instead, you should just delete the ignored directory when it shows up in its > parent (then you can just directory compare directory names without having to > mess around with normpath and basename). > > # Do not descend into ignored directories. > for i in range(len(dirnames) - 1, -1, -1): > if dirnames[i] in ignore_directories: > del dirnames[i] > > I think this is the intended usage of os.walk(topdown=true). > > https://codereview.chromium.org/1662833005/diff/80001/src/end_to_end_test.py > File src/end_to_end_test.py (right): > > https://codereview.chromium.org/1662833005/diff/80001/src/end_to_end_test.py#... > src/end_to_end_test.py:43: class TestEndToEndConvert(unittest.TestCase): > Why no longer using TestCaseWithTempDir? > > https://codereview.chromium.org/1662833005/diff/80001/src/end_to_end_test.py#... > src/end_to_end_test.py:79: output = subprocess.check_output( > It's really quite strange that you run the entire test in setUp (now > setUpClass), and just do checking in the individual test methods. > > But for performance reasons, I suppose this makes sense to do it. > > However, this shouldn't be done in this CL. This CL is fixing a major bug --- > Caterpillar is broken and the tests aren't testing it. So you shouldn't be > refactoring or fixing test performance here. Please do this later. > > Come to think of it, you should probably save the get_relative_filepaths > refactor for that cleanup CL as well. Alright, I'll revert the refactoring etc. and just fix the bugs here. Then I'll upload separate CLs for the other things.
Description was changed from ========== Updates end-to-end tests with report. Fixes report merging bugs. This CL updates the report linking based on recent changes to master, and fixes a bug that was missed due to the lack of end-to-end tests for report linking. ========== to ========== Fixes report merging bugs. This CL updates the report linking based on recent changes to master, and fixes a bug that was missed due to the lack of end-to-end tests for report linking. This stops Caterpillar from crashing when trying to generate a report. Tests for this will be added in a separate CL. ==========
Description was changed from ========== Fixes report merging bugs. This CL updates the report linking based on recent changes to master, and fixes a bug that was missed due to the lack of end-to-end tests for report linking. This stops Caterpillar from crashing when trying to generate a report. Tests for this will be added in a separate CL. ========== to ========== Fixes report merging bugs. This CL updates the report linking based on recent changes to master, and fixes a bug that was missed due to the lack of end-to-end tests for report linking. This stops Caterpillar from crashing when trying to generate a report. Tests to catch the problem if it arises again in future are also added in this CL. ==========
Reverted most changes in line with your comments.
LGTM but still the CL description needs to be more obvious. File a bug in GitHub. Basically Caterpillar crashes every time at the moment so the bug should be "Caterpillar crashes generating report" and the fix should be titled "Fix crash generating report". Then you can go into details about it in the CL description body.
Description was changed from ========== Fixes report merging bugs. This CL updates the report linking based on recent changes to master, and fixes a bug that was missed due to the lack of end-to-end tests for report linking. This stops Caterpillar from crashing when trying to generate a report. Tests to catch the problem if it arises again in future are also added in this CL. ========== to ========== Fixes Caterpillar crashing while generating report. Caterpillar currently crashes while generating a report. This is due to API changes between writing and merging the report linking branch. This CL updates the report code to account for the new API and new variable names. Tests to catch the problem if it arises again in future are also added in this CL. ==========
Description was changed from ========== Fixes Caterpillar crashing while generating report. Caterpillar currently crashes while generating a report. This is due to API changes between writing and merging the report linking branch. This CL updates the report code to account for the new API and new variable names. Tests to catch the problem if it arises again in future are also added in this CL. ========== to ========== Fixes Caterpillar crashing while generating report. Caterpillar currently crashes while generating a report. This is due to API changes between writing and merging the report linking branch. This CL updates the report code to account for the new API and new variable names. Tests to catch the problem if it arises again in future are also added in this CL. Resolves #42. https://github.com/chromium/caterpillar/issues/42 ==========
Description was changed from ========== Fixes Caterpillar crashing while generating report. Caterpillar currently crashes while generating a report. This is due to API changes between writing and merging the report linking branch. This CL updates the report code to account for the new API and new variable names. Tests to catch the problem if it arises again in future are also added in this CL. Resolves #42. https://github.com/chromium/caterpillar/issues/42 ========== to ========== Fixes Caterpillar crashing while generating report. Caterpillar currently crashes while generating a report. This is due to API changes between writing and merging the report linking branch. This CL updates the report code to account for the new API and new variable names. Tests to catch the problem if it arises again in future are also added in this CL. Resolves #42. https://github.com/chromium/caterpillar/issues/42 R=mgiuca@chromium.org Committed: dc0b8af616f016fe9fa3e9eeeea4b12b709d69c3 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) manually as dc0b8af616f016fe9fa3e9eeeea4b12b709d69c3 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
