|
|
Created:
3 years, 7 months ago by Shimi Zhang Modified:
3 years, 7 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExclude crash tests for O
BUG=714672
Review-Url: https://codereview.chromium.org/2846703004
Cr-Commit-Position: refs/heads/master@{#468253}
Committed: https://chromium.googlesource.com/chromium/src/+/ef4a5d5cee27a964cdfbe498ae71a93761b78b5b
Patch Set 1 #Patch Set 2 : Add documentation for later on cleanup #
Total comments: 2
Patch Set 3 : Rephrase comments #
Total comments: 1
Patch Set 4 : nits #Patch Set 5 : use android_sdk_version #
Messages
Total messages: 43 (20 generated)
The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ctzsm@chromium.org changed reviewers: + agrieve@chromium.org, dfalcantara@chromium.org, isherman@chromium.org, wnwen@chromium.org
Just Exclude crush tests for O, then we need to bring them back when O SDK rolls.
So does this mean the other two CLs won't land?
On 2017/04/27 21:43:10, slow (dfalcantara) wrote: > So does this mean the other two CLs won't land? They won't be landed if we go with this way.
I'll defer to the folks who are most familiar with the build system then.
Hmm, I'm a bit worried that we'll forget about this and lose test coverage; I'd prefer a solution that addresses the build error instead. That said, I am far from expert in the realm of Android builds, and will defer to others who are more familiar with the tradeoffs. Also, nit: s/crush/crash in the CL description (and title)
Description was changed from ========== Exclude crush tests for O BUG=714672 ========== to ========== Exclude crash tests for O BUG=714672 ==========
On 2017/04/27 21:53:37, Ilya Sherman wrote: > Hmm, I'm a bit worried that we'll forget about this and lose test coverage; I'd > prefer a solution that addresses the build error instead. That said, I am far > from expert in the realm of Android builds, and will defer to others who are > more familiar with the tradeoffs. > It won't compile when we do SDK roll, so we will know it, we could also create a release blocker crbug to track it. > Also, nit: s/crush/crash in the CL description (and title) Oops, fixed.
On 2017/04/27 21:57:53, Shimi Zhang wrote: > On 2017/04/27 21:53:37, Ilya Sherman wrote: > > Hmm, I'm a bit worried that we'll forget about this and lose test coverage; > I'd > > prefer a solution that addresses the build error instead. That said, I am far > > from expert in the realm of Android builds, and will defer to others who are > > more familiar with the tradeoffs. > > > It won't compile when we do SDK roll, so we will know it, we could also create a > release blocker crbug to track it. If the compile will actually fail once the SDK is rolled, I'm sort of ok with it. I guess my main concern is that someone will see a failure or a release blocker bug and basically punt it, e.g. disable the tests in some other way. I'd recommend at least adding some docs to clarify what the expected fix is.
On 2017/04/27 21:59:56, Ilya Sherman wrote: > On 2017/04/27 21:57:53, Shimi Zhang wrote: > > On 2017/04/27 21:53:37, Ilya Sherman wrote: > > > Hmm, I'm a bit worried that we'll forget about this and lose test coverage; > > I'd > > > prefer a solution that addresses the build error instead. That said, I am > far > > > from expert in the realm of Android builds, and will defer to others who are > > > more familiar with the tradeoffs. > > > > > It won't compile when we do SDK roll, so we will know it, we could also create > a > > release blocker crbug to track it. > > If the compile will actually fail once the SDK is rolled, I'm sort of ok with > it. I guess my main concern is that someone will see a failure or a release > blocker bug and basically punt it, e.g. disable the tests in some other way. > I'd recommend at least adding some docs to clarify what the expected fix is. I think when we roll O SDK, use_really_unpublished_api's won't be meaningful anymore, so tests will automatically be included, but why would it stop compiling?.. Himm, yeah so the definition of TestJobScheduler won't be correct anymore (will need the new methods). It will not be immediately obvious that this CL will be reverted though, so documentation is key.
On 2017/04/27 22:18:01, sgurun wrote: > On 2017/04/27 21:59:56, Ilya Sherman wrote: > > On 2017/04/27 21:57:53, Shimi Zhang wrote: > > > On 2017/04/27 21:53:37, Ilya Sherman wrote: > > > > Hmm, I'm a bit worried that we'll forget about this and lose test > coverage; > > > I'd > > > > prefer a solution that addresses the build error instead. That said, I am > > far > > > > from expert in the realm of Android builds, and will defer to others who > are > > > > more familiar with the tradeoffs. > > > > > > > It won't compile when we do SDK roll, so we will know it, we could also > create > > a > > > release blocker crbug to track it. > > > > If the compile will actually fail once the SDK is rolled, I'm sort of ok with > > it. I guess my main concern is that someone will see a failure or a release > > blocker bug and basically punt it, e.g. disable the tests in some other way. > > I'd recommend at least adding some docs to clarify what the expected fix is. > > > I think when we roll O SDK, use_really_unpublished_api's won't be meaningful > anymore, so tests will automatically be included, but why would it stop > compiling?.. Himm, yeah so the definition of TestJobScheduler won't be correct > anymore (will need the new methods). It will not be immediately obvious that Yeah, that's how I think it won't compile. > this CL will be reverted though, so documentation is key. I am going to create a crbug, and refer that crbug in both test files and this BUILD.gn.
On 2017/04/27 22:33:58, Shimi Zhang wrote: > On 2017/04/27 22:18:01, sgurun wrote: > > On 2017/04/27 21:59:56, Ilya Sherman wrote: > > > On 2017/04/27 21:57:53, Shimi Zhang wrote: > > > > On 2017/04/27 21:53:37, Ilya Sherman wrote: > > > > > Hmm, I'm a bit worried that we'll forget about this and lose test > > coverage; > > > > I'd > > > > > prefer a solution that addresses the build error instead. That said, I > am > > > far > > > > > from expert in the realm of Android builds, and will defer to others who > > are > > > > > more familiar with the tradeoffs. > > > > > > > > > It won't compile when we do SDK roll, so we will know it, we could also > > create > > > a > > > > release blocker crbug to track it. > > > > > > If the compile will actually fail once the SDK is rolled, I'm sort of ok > with > > > it. I guess my main concern is that someone will see a failure or a release > > > blocker bug and basically punt it, e.g. disable the tests in some other way. > > > > I'd recommend at least adding some docs to clarify what the expected fix is. > > > > > > I think when we roll O SDK, use_really_unpublished_api's won't be meaningful > > anymore, so tests will automatically be included, but why would it stop > > compiling?.. Himm, yeah so the definition of TestJobScheduler won't be correct > > anymore (will need the new methods). It will not be immediately obvious that > > Yeah, that's how I think it won't compile. > > > this CL will be reverted though, so documentation is key. > > I am going to create a crbug, and refer that crbug in both test files and this > BUILD.gn. not owner but it looks the most reasonable way to go, to me.
LGTM % a comment about phrasing, assuming that this is the preferred solution by folks who know the build well. https://codereview.chromium.org/2846703004/diff/20001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2846703004/diff/20001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:415: # TODO(crbug/716236): Remove it and update those two files after SDK rolls. nit: I'd rephrase this to something like "Remove this exclusion, and update these two test files, after the O SDK is rolled."
https://codereview.chromium.org/2846703004/diff/20001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2846703004/diff/20001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:415: # TODO(crbug/716236): Remove it and update those two files after SDK rolls. On 2017/04/27 23:35:37, Ilya Sherman wrote: > nit: I'd rephrase this to something like "Remove this exclusion, and update > these two test files, after the O SDK is rolled." Done.
just a nit build-wise lgtm https://codereview.chromium.org/2846703004/diff/40001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2846703004/diff/40001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:417: if (defined(use_really_unpublished_apis) && use_really_unpublished_apis) { nit: would be better to not use downstream gn arg here. Instead, use: if (android_sdk_version != "O")
The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/28 01:05:33, agrieve wrote: > just a nit build-wise lgtm > > https://codereview.chromium.org/2846703004/diff/40001/chrome/android/BUILD.gn > File chrome/android/BUILD.gn (right): > > https://codereview.chromium.org/2846703004/diff/40001/chrome/android/BUILD.gn... > chrome/android/BUILD.gn:417: if (defined(use_really_unpublished_apis) && > use_really_unpublished_apis) { > nit: would be better to not use downstream gn arg here. Instead, use: > > if (android_sdk_version != "O") Replaced use_really_unpublished_apis with android_sdk_version, just want to confirm that we will have android_sdk_version change to a version number other than "O" when we roll SDK, otherwise this won't break compilation.
On 2017/04/28 17:27:29, Shimi Zhang wrote: > On 2017/04/28 01:05:33, agrieve wrote: > > just a nit build-wise lgtm > > > > https://codereview.chromium.org/2846703004/diff/40001/chrome/android/BUILD.gn > > File chrome/android/BUILD.gn (right): > > > > > https://codereview.chromium.org/2846703004/diff/40001/chrome/android/BUILD.gn... > > chrome/android/BUILD.gn:417: if (defined(use_really_unpublished_apis) && > > use_really_unpublished_apis) { > > nit: would be better to not use downstream gn arg here. Instead, use: > > > > if (android_sdk_version != "O") > > Replaced use_really_unpublished_apis with android_sdk_version, just want to > confirm that we will have android_sdk_version change to a version number other > than "O" when we roll SDK, otherwise this won't break compilation. Confirmed that with agrieve@.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ctzsm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ctzsm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2846703004/#ps80001 (title: "use android_sdk_version")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
dfalcantara@, could you please check those two test files, forgot to ask file owner to review.
lgtm
The CQ bit was checked by ctzsm@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by ctzsm@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493530734935590, "parent_rev": "d38126b6c8415249dd4a8d347d47c13d06831c0d", "commit_rev": "ef4a5d5cee27a964cdfbe498ae71a93761b78b5b"}
Message was sent while issue was closed.
Description was changed from ========== Exclude crash tests for O BUG=714672 ========== to ========== Exclude crash tests for O BUG=714672 Review-Url: https://codereview.chromium.org/2846703004 Cr-Commit-Position: refs/heads/master@{#468253} Committed: https://chromium.googlesource.com/chromium/src/+/ef4a5d5cee27a964cdfbe498ae71... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ef4a5d5cee27a964cdfbe498ae71... |