Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15809007:
Clean up browser tests in Files.app. (Closed)
|
Created:
7 years, 6 months ago by mtomasz Modified:
7 years, 6 months ago CC:
chromium-reviews, Aaron Boodman, rginda+watch_chromium.org, chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClean up browser tests in Files.app.
This patch cleans up tests on the Javascript side for Files.app:
- Introduce a StepRunner, which runs multi-step test cases.
- Add checking for Javascript errors after each test case.
- Add missing comments.
TEST=browser_tests
BUG=244327
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203123
Patch Set 1 #
Total comments: 8
Patch Set 2 : Addressed comments. #
Total comments: 6
Patch Set 3 : Addressed further comments. #
Messages
Total messages: 14 (0 generated)
@hashimoto, @hirono: PTAL. Thanks.
On 2013/05/30 02:25:40, mtomasz wrote: > @hashimoto, @hirono: PTAL. Thanks. I'm really like StepsRunner. It's very interesting to me. Thank you!
https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js (right): https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:59: this.steps_.push(function() {}); How about putting 'checkIfNoErrorsOccured' here? https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js (right): https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:172: steps.run([ steps.run may be called multiple times by users of StepsRunner, where the StepsRunner does not work currently. How about introducing helper function runSteps(steps)? runSteps([ function() { setupAndWaitUntilReady(path, this.next); }, function(inAppId, actualFilesBefore) { ... } ]);
https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js (right): https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:59: this.steps_.push(function() {}); On 2013/05/30 04:46:10, hirono wrote: > How about putting 'checkIfNoErrorsOccured' here? I was thinking about, however I thought that putting asserts here would mess up the code. This is just a very simple steps runner, not even in test_cases.js file. WDYT? https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js (right): https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:172: steps.run([ On 2013/05/30 04:46:10, hirono wrote: > steps.run may be called multiple times by users of StepsRunner, where the > StepsRunner does not work currently. > How about introducing helper function runSteps(steps)? > > runSteps([ > function() { > setupAndWaitUntilReady(path, this.next); > }, > function(inAppId, actualFilesBefore) { > ... > } > ]); Great idea. Done.
Thank you! https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js (right): https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:59: this.steps_.push(function() {}); I'm not sure, but StepsRunner already have the role of assertion since it uses callbackPass. If StepsRunner doesn't call callbackPass, this class may lose the value as a helper class. So it is not good idea to remove callbackPass. And If StepsRunner should call callbackPass, it is natural to do other assertion. But it is my thought. If StepsRunner does not call checkIfNoErrorsOccured, still lgtm. Thank you for replying. On 2013/05/30 06:40:53, mtomasz wrote: > On 2013/05/30 04:46:10, hirono wrote: > > How about putting 'checkIfNoErrorsOccured' here? > > I was thinking about, however I thought that putting asserts here would mess up > the code. This is just a very simple steps runner, not even in test_cases.js > file. WDYT?
https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js (right): https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:59: this.steps_.push(function() {}); On 2013/05/30 07:08:59, hirono wrote: > I'm not sure, but StepsRunner already have the role of assertion since it uses > callbackPass. > If StepsRunner doesn't call callbackPass, this class may lose the value as a > helper class. > So it is not good idea to remove callbackPass. > And If StepsRunner should call callbackPass, it is natural to do other > assertion. > But it is my thought. > If StepsRunner does not call checkIfNoErrorsOccured, still lgtm. > Thank you for replying. > > On 2013/05/30 06:40:53, mtomasz wrote: > > On 2013/05/30 04:46:10, hirono wrote: > > > How about putting 'checkIfNoErrorsOccured' here? > > > > I was thinking about, however I thought that putting asserts here would mess > up > > the code. This is just a very simple steps runner, not even in test_cases.js > > file. WDYT? > Hm, callbackPass is just a utility to make tests work asynchronously. I think it is different than a real assert. Also I think that StepsRunner sounds like a tool to run steps only. Users may not expect that it does some extra calls via ipc and validates errors. However, this is also only my thought. Let's wait for hashimoto's point of view.
lgtm https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js (right): https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:59: this.steps_.push(function() {}); On 2013/05/30 07:30:11, mtomasz wrote: > On 2013/05/30 07:08:59, hirono wrote: > > I'm not sure, but StepsRunner already have the role of assertion since it uses > > callbackPass. > > If StepsRunner doesn't call callbackPass, this class may lose the value as a > > helper class. > > So it is not good idea to remove callbackPass. > > And If StepsRunner should call callbackPass, it is natural to do other > > assertion. > > But it is my thought. > > If StepsRunner does not call checkIfNoErrorsOccured, still lgtm. > > Thank you for replying. > > > > On 2013/05/30 06:40:53, mtomasz wrote: > > > On 2013/05/30 04:46:10, hirono wrote: > > > > How about putting 'checkIfNoErrorsOccured' here? > > > > > > I was thinking about, however I thought that putting asserts here would mess > > up > > > the code. This is just a very simple steps runner, not even in test_cases.js > > > file. WDYT? > > > > Hm, callbackPass is just a utility to make tests work asynchronously. I think it > is different than a real assert. Also I think that StepsRunner sounds like a > tool to run steps only. Users may not expect that it does some extra calls via > ipc and validates errors. > > However, this is also only my thought. Let's wait for hashimoto's point of view. I feel neutral about this point. Making StepsRunner focusing on its own job sounds good. OTH, chrome.test.callback which is used to implement callbackPass seems doing some error check. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... Maybe we can implement more comprehensive test fixture implemented with StepsRunner in near future to reduce code duplication like "appId = inAppId" pattern, then we can include checkIfNoErrorsOccured in it. https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js (right): https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:67: this.steps_.push(function() {}); nit: Please add a comment about the role of this empty function. https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js (right): https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:178: appId = inAppId; Can we make this setupAndWaitUntilReady call and appId storing code shared between all test cases in future? https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:429: }; thanks you for fixing my mistake :)
https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js (right): https://codereview.chromium.org/15809007/diff/1/chrome/test/data/extensions/a... chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:59: this.steps_.push(function() {}); On 2013/05/30 08:21:45, hashimoto wrote: > On 2013/05/30 07:30:11, mtomasz wrote: > > On 2013/05/30 07:08:59, hirono wrote: > > > I'm not sure, but StepsRunner already have the role of assertion since it > uses > > > callbackPass. > > > If StepsRunner doesn't call callbackPass, this class may lose the value as a > > > helper class. > > > So it is not good idea to remove callbackPass. > > > And If StepsRunner should call callbackPass, it is natural to do other > > > assertion. > > > But it is my thought. > > > If StepsRunner does not call checkIfNoErrorsOccured, still lgtm. > > > Thank you for replying. > > > > > > On 2013/05/30 06:40:53, mtomasz wrote: > > > > On 2013/05/30 04:46:10, hirono wrote: > > > > > How about putting 'checkIfNoErrorsOccured' here? > > > > > > > > I was thinking about, however I thought that putting asserts here would > mess > > > up > > > > the code. This is just a very simple steps runner, not even in > test_cases.js > > > > file. WDYT? > > > > > > > Hm, callbackPass is just a utility to make tests work asynchronously. I think > it > > is different than a real assert. Also I think that StepsRunner sounds like a > > tool to run steps only. Users may not expect that it does some extra calls via > > ipc and validates errors. > > > > However, this is also only my thought. Let's wait for hashimoto's point of > view. > > I feel neutral about this point. > Making StepsRunner focusing on its own job sounds good. > OTH, chrome.test.callback which is used to implement callbackPass seems doing > some error check. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... > > Maybe we can implement more comprehensive test fixture implemented with > StepsRunner in near future to reduce code duplication like "appId = inAppId" > pattern, then we can include checkIfNoErrorsOccured in it. That's a good idea. I will write a utility which adds the first step (initialization) and the last step (error checking) in the nearest future. https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/file_manager_browsertest/background.js (right): https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/file_manager_browsertest/background.js:67: this.steps_.push(function() {}); On 2013/05/30 08:21:45, hashimoto wrote: > nit: Please add a comment about the role of this empty function. Done. https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... File chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js (right): https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:178: appId = inAppId; On 2013/05/30 08:21:45, hashimoto wrote: > Can we make this setupAndWaitUntilReady call and appId storing code shared > between all test cases in future? Let's do that in a separate cl. https://codereview.chromium.org/15809007/diff/11001/chrome/test/data/extensio... chrome/test/data/extensions/api_test/file_manager_browsertest/test_cases.js:429: }; On 2013/05/30 08:21:45, hashimoto wrote: > thanks you for fixing my mistake :) np :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/15809007/18001
On 2013/05/30 08:51:53, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/mtomasz%40chromium.org/15809007/18001 I don't see any difference between Patch Set 2 and 3, could you try uploading again?
On 2013/05/30 08:54:36, hashimoto wrote: > On 2013/05/30 08:51:53, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/mtomasz%2540chromium.org/15809007/18001 > > I don't see any difference between Patch Set 2 and 3, could you try uploading > again? Fixed, PTAL.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/15809007/20002
lgtm thanks
Message was sent while issue was closed.
Change committed as 203123 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
