Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2950353002:
[Payments] Avoid a crash caused by AddressValidatorTest. (Closed)
DescriptionProblem:
AddressValidatorTest.SubKeysNotLoaded would crash,
because AddressValidator would not handle the synchronous cases.
The DCHECK in ThreadTaskRunnerHandle::Get() causes the crash,
and it should.
Solution:
Since the process of loading rules should always be asynchronous,
there should be no check in the RulesLoaded, the later DCHECK
would serve the purpose.
Therefore, change the unit-test country code from "ZZ" to "OZ",
because the data for ZZ can't be parsed, and the success = 0,
and since the tests are synchronous, there would be a crash.
For "OZ", there is no data, therefore success = 1, but there would
be no rules loaded for that, which serves the purpose.
ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed,
because for the case of AddressValidator::RULES_UNAVAILABLE,
success == true, but status != AddressValidator::SUCCESS.
Also, since the AddressValidatorTest loads rules synchronously,
success should always be true, or there will be a crash.
Add an extra check in this unittest, to make sure that the rules
are not loaded.
Rename SubKeysNotLoaded to SubKeysNotExists,
to be clear that no data exists for this country code.
BUG=735036
Review-Url: https://codereview.chromium.org/2950353002
Cr-Commit-Position: refs/heads/master@{#483488}
Committed: https://chromium.googlesource.com/chromium/src/+/1ccce34079720f94e410b5409e601b9d30ebe09b
Patch Set 1 : No retry for synchronous. #Patch Set 2 : Rename. Re-example. #
Total comments: 4
Patch Set 3 : Change the damn line 78. #Patch Set 4 : Crash if you have to. (Undo Patch Set 1.) #
Total comments: 1
Patch Set 5 : Remove a comment. #
Total comments: 4
Patch Set 6 : Nit. #Messages
Total messages: 90 (72 generated)
The CQ bit was checked by parastoog@google.com 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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by parastoog@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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== No retry for synchronous. BUG= ========== to ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysEmpty, because the SubKeys are loaded, but they are empty. BUG=735036 ==========
The CQ bit was checked by parastoog@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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
parastoog@chromium.org changed reviewers: + mathp@chromium.org
mathp@chromium.org: Please review changes in
Description was changed from ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysEmpty, because the SubKeys are loaded, but they are empty. BUG=735036 ========== to ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysEmpty, because the SubKeys are loaded, but they are empty. BUG=735036 ==========
The CQ bit was checked by parastoog@chromium.org
The CQ bit was checked by parastoog@google.com 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...
Patchset #2 (id:2) has been deleted
Description was changed from ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysEmpty, because the SubKeys are loaded, but they are empty. BUG=735036 ========== to ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. Also, change the example from "ZZ" to "OZ", because in the countryinfo.txt, there IS data for "ZZ", but the key "ZZ" is not listed in the country codes. This change is to avoid confusion. BUG=735036 ==========
The CQ bit was checked by parastoog@google.com 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 parastoog@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
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-chromium-committers". Note that this has nothing to do with OWNERS files.
mathp@chromium.org changed reviewers: + sebsg@chromium.org - parastoog@chromium.org
+Seb because he's been looking at synchronous
https://codereview.chromium.org/2950353002/diff/30001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2950353002/diff/30001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.cc:167: if (base::ThreadTaskRunnerHandle::IsSet()) { I don't think we should modify the production code for the sake of the test. I think you fixed the crash by loading "OZ", and it make much more sense for that test. If you do this the test will still fail though, because in OnAddressValidationRulesLoaded of the test class, it expects the rules to be loaded successfully. You'll need to change that assumption to depend on a variable, since in the case of this test is should be RULES_UNAVAILABLE. I'd be much more comfortable with that change since it only affects the test class. Thanks!
parastoog@chromium.org changed reviewers: + parastoog@chromium.org
https://codereview.chromium.org/2950353002/diff/30001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2950353002/diff/30001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.cc:167: if (base::ThreadTaskRunnerHandle::IsSet()) { On 2017/06/23 17:26:04, sebsg wrote: > I don't think we should modify the production code for the sake of the test. Anyway, if it's synchronous, the next line would crash, and if it's async, this line would not make a difference. But at the end, if you insist that it's not a good design, we can remove the test. > think you fixed the crash by loading "OZ", and it make much more sense for that > test. No. The crash was fixed by adding this line. That change was introduced later (when I was taking a look at countryinfo.txt out of curiosity, and noticed inconsistent data on "ZZ", and changed this to avoid confusion. Also, because OZ reminds me of the "Wizard of Oz" ;) ) > If you do this the test will still fail though, because in > OnAddressValidationRulesLoaded of the test class, it expects the rules to be > loaded successfully. No. The OnAddressValidationRulesLoaded checks for ASSERT_EQ(success, status == AddressValidator::SUCCESS) and NOT for ASSERT_EQ(success, AddressValidator::SUCCESS). which is mostly for the sake of testing Validate(). This class doesn't require success == 1. I was confused for a good while by this line, the tests passing, and success = 0! You'll need to change that assumption to depend on a > variable, since in the case of this test is should be RULES_UNAVAILABLE. I'd be > much more comfortable with that change since it only affects the test class. > > Thanks
https://codereview.chromium.org/2950353002/diff/30001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2950353002/diff/30001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.cc:167: if (base::ThreadTaskRunnerHandle::IsSet()) { On 2017/06/23 20:30:11, Hirondelle wrote: > On 2017/06/23 17:26:04, sebsg wrote: > > I don't think we should modify the production code for the sake of the test. > > Anyway, if it's synchronous, the next line would crash, and if it's async, this > line would not make a difference. But at the end, if you insist that it's not a > good design, we can remove the test. > > > think you fixed the crash by loading "OZ", and it make much more sense for > that > > test. > > No. The crash was fixed by adding this line. That change was introduced later > (when I was taking a look at countryinfo.txt out of curiosity, and noticed > inconsistent data on "ZZ", and changed this to avoid confusion. Also, because OZ > reminds me of the "Wizard of Oz" ;) ) > > > If you do this the test will still fail though, because in > > OnAddressValidationRulesLoaded of the test class, it expects the rules to be > > loaded successfully. > > No. The OnAddressValidationRulesLoaded checks for > ASSERT_EQ(success, status == AddressValidator::SUCCESS) and NOT for > ASSERT_EQ(success, AddressValidator::SUCCESS). > > which is mostly for the sake of testing Validate(). This class doesn't require > success == 1. > I was confused for a good while by this line, the tests passing, and success = > 0! > > You'll need to change that assumption to depend on a > > variable, since in the case of this test is should be RULES_UNAVAILABLE. I'd > be > > much more comfortable with that change since it only affects the test class. > > > > Thanks > What I suggest is changing the expected status for the test. Right now it's hardcoded (line 82) but you can change it. You can make it be RulesUnavailable for the SubKeysNotExist test. It seems like it works on my machine.
The CQ bit was checked by parastoog@google.com 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.
Patchset #3 (id:50001) has been deleted
The CQ bit was checked by parastoog@google.com
The CQ bit was unchecked by parastoog@google.com
The CQ bit was checked by parastoog@google.com 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...
parastoog@google.com changed reviewers: - parastoog@chromium.org
https://codereview.chromium.org/2950353002/diff/30001/third_party/libaddressi... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2950353002/diff/30001/third_party/libaddressi... third_party/libaddressinput/chromium/chrome_address_validator.cc:167: if (base::ThreadTaskRunnerHandle::IsSet()) { On 2017/06/23 21:59:32, sebsg wrote: > On 2017/06/23 20:30:11, Hirondelle wrote: > > On 2017/06/23 17:26:04, sebsg wrote: > > > I don't think we should modify the production code for the sake of the test. > > > > Anyway, if it's synchronous, the next line would crash, and if it's async, > this > > line would not make a difference. But at the end, if you insist that it's not > a > > good design, we can remove the test. > > > > > think you fixed the crash by loading "OZ", and it make much more sense for > > that > > > test. > > > > No. The crash was fixed by adding this line. That change was introduced later > > (when I was taking a look at countryinfo.txt out of curiosity, and noticed > > inconsistent data on "ZZ", and changed this to avoid confusion. Also, because > OZ > > reminds me of the "Wizard of Oz" ;) ) > > > > > If you do this the test will still fail though, because in > > > OnAddressValidationRulesLoaded of the test class, it expects the rules to be > > > loaded successfully. > > > > No. The OnAddressValidationRulesLoaded checks for > > ASSERT_EQ(success, status == AddressValidator::SUCCESS) and NOT for > > ASSERT_EQ(success, AddressValidator::SUCCESS). > > > > which is mostly for the sake of testing Validate(). This class doesn't require > > success == 1. > > I was confused for a good while by this line, the tests passing, and success = > > 0! > > > > You'll need to change that assumption to depend on a > > > variable, since in the case of this test is should be RULES_UNAVAILABLE. > I'd > > be > > > much more comfortable with that change since it only affects the test class. > > > > > > Thanks > > > > What I suggest is changing the expected status for the test. Right now it's > hardcoded (line 82) but you can change it. You can make it be RulesUnavailable > for the SubKeysNotExist test. It seems like it works on my machine. Done.
Description was changed from ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. Also, change the example from "ZZ" to "OZ", because in the countryinfo.txt, there IS data for "ZZ", but the key "ZZ" is not listed in the country codes. This change is to avoid confusion. BUG=735036 ========== to ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. Add an extra check in this unittest, to make sure that the rules are not loaded. Also, change the example from "ZZ" to "OZ", because in the countryinfo.txt, there IS data for "ZZ", but the key "ZZ" is not listed in the country codes. This change is ONLY to avoid confusion. BUG=735036 ==========
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 parastoog@google.com 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...
Description was changed from ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. Add an extra check in this unittest, to make sure that the rules are not loaded. Also, change the example from "ZZ" to "OZ", because in the countryinfo.txt, there IS data for "ZZ", but the key "ZZ" is not listed in the country codes. This change is ONLY to avoid confusion. BUG=735036 ========== to ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. Add an extra check in this unittest, to make sure that the rules are not loaded. Also, change the example from "ZZ" to "OZ", because in the countryinfo.txt, there IS data for "ZZ", but the key "ZZ" is not listed in the country codes. This change is ONLY to avoid confusion. ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed, because for the case of AddressValidator::RULES_UNAVAILABLE, success == true, but status != AddressValidator::SUCCESS. BUG=735036 ==========
The CQ bit was checked by parastoog@google.com 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...
Patchset #4 (id:90001) has been deleted
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 parastoog@google.com 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...
Description was changed from ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. In AddressValidator, do not retry loading the rules for synchronous cases (= AddressValidator unit tests.) Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. Add an extra check in this unittest, to make sure that the rules are not loaded. Also, change the example from "ZZ" to "OZ", because in the countryinfo.txt, there IS data for "ZZ", but the key "ZZ" is not listed in the country codes. This change is ONLY to avoid confusion. ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed, because for the case of AddressValidator::RULES_UNAVAILABLE, success == true, but status != AddressValidator::SUCCESS. BUG=735036 ========== to ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. The DCHECK causes the crash, and it should. Also, change the unit-test country code from "ZZ" to "OZ", because the data for ZZ can't be parsed, and the success = 0, and since the tests are synchronous, there would be a crash. For "OZ", there is no data, therefore success = 1, but there would be no rules for that, which serves the purpose. ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed, because for the case of AddressValidator::RULES_UNAVAILABLE, success == true, but status != AddressValidator::SUCCESS. Add an extra check in this unittest, to make sure that the rules are not loaded. Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. BUG=735036 ==========
Description was changed from ========== AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. The DCHECK causes the crash, and it should. Also, change the unit-test country code from "ZZ" to "OZ", because the data for ZZ can't be parsed, and the success = 0, and since the tests are synchronous, there would be a crash. For "OZ", there is no data, therefore success = 1, but there would be no rules for that, which serves the purpose. ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed, because for the case of AddressValidator::RULES_UNAVAILABLE, success == true, but status != AddressValidator::SUCCESS. Add an extra check in this unittest, to make sure that the rules are not loaded. Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. BUG=735036 ========== to ========== Problem: AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. The DCHECK in ThreadTaskRunnerHandle::Get() causes the crash, and it should. Solution: Since the process of loading rules should always be asynchronous, there should be no check in the RulesLoaded, the later DCHECK would serve the purpose. Therefore, change the unit-test country code from "ZZ" to "OZ", because the data for ZZ can't be parsed, and the success = 0, and since the tests are synchronous, there would be a crash. For "OZ", there is no data, therefore success = 1, but there would be no rules loaded for that, which serves the purpose. ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed, because for the case of AddressValidator::RULES_UNAVAILABLE, success == true, but status != AddressValidator::SUCCESS. Add an extra check in this unittest, to make sure that the rules are not loaded. Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. BUG=735036 ==========
Patchset #3 (id:70001) has been deleted
lgtm % small comment. Good job! https://codereview.chromium.org/2950353002/diff/120001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator.cc (right): https://codereview.chromium.org/2950353002/diff/120001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator.cc:166: // No need to retry if it's synchronous (which means that it's a test.) Remove comment? :)
The CQ bit was checked by parastoog@google.com 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 checked by parastoog@google.com 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...
Patchset #5 (id:140001) has been deleted
lgtm with nits https://codereview.chromium.org/2950353002/diff/160001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc (right): https://codereview.chromium.org/2950353002/diff/160001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:82: ASSERT_EQ(success, status == expected_status_); I think the expected value usually comes first in ASSERT_EQ? https://codereview.chromium.org/2950353002/diff/160001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:131: TEST_F(AddressValidatorTest, SubKeysNotExists) { How about SubKeys_NonExistent
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Problem: AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. The DCHECK in ThreadTaskRunnerHandle::Get() causes the crash, and it should. Solution: Since the process of loading rules should always be asynchronous, there should be no check in the RulesLoaded, the later DCHECK would serve the purpose. Therefore, change the unit-test country code from "ZZ" to "OZ", because the data for ZZ can't be parsed, and the success = 0, and since the tests are synchronous, there would be a crash. For "OZ", there is no data, therefore success = 1, but there would be no rules loaded for that, which serves the purpose. ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed, because for the case of AddressValidator::RULES_UNAVAILABLE, success == true, but status != AddressValidator::SUCCESS. Add an extra check in this unittest, to make sure that the rules are not loaded. Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. BUG=735036 ========== to ========== Problem: AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. The DCHECK in ThreadTaskRunnerHandle::Get() causes the crash, and it should. Solution: Since the process of loading rules should always be asynchronous, there should be no check in the RulesLoaded, the later DCHECK would serve the purpose. Therefore, change the unit-test country code from "ZZ" to "OZ", because the data for ZZ can't be parsed, and the success = 0, and since the tests are synchronous, there would be a crash. For "OZ", there is no data, therefore success = 1, but there would be no rules loaded for that, which serves the purpose. ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed, because for the case of AddressValidator::RULES_UNAVAILABLE, success == true, but status != AddressValidator::SUCCESS. Also, since the AddressValidatorTest loads rules synchronously, success should always be true, or there will be a crash. Add an extra check in this unittest, to make sure that the rules are not loaded. Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. BUG=735036 ==========
The CQ bit was checked by parastoog@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org, mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2950353002/#ps180001 (title: "Typos and Comments.")
The CQ bit was checked by parastoog@google.com 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...
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by parastoog@google.com 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...
Patchset #6 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2950353002/diff/160001/third_party/libaddress... File third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc (right): https://codereview.chromium.org/2950353002/diff/160001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:82: ASSERT_EQ(success, status == expected_status_); On 2017/06/29 19:11:25, Mathieu wrote: > I think the expected value usually comes first in ASSERT_EQ? Done. https://codereview.chromium.org/2950353002/diff/160001/third_party/libaddress... third_party/libaddressinput/chromium/chrome_address_validator_unittest.cc:131: TEST_F(AddressValidatorTest, SubKeysNotExists) { On 2017/06/29 19:11:25, Mathieu wrote: > How about SubKeys_NonExistent Done.
The CQ bit was checked by parastoog@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, sebsg@chromium.org Link to the patchset: https://codereview.chromium.org/2950353002/#ps220001 (title: "Nit.")
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": 220001, "attempt_start_ts": 1498770814615030,
"parent_rev": "505b8e62281ad25172bfa025bbd546ec8a3c955c", "commit_rev":
"1ccce34079720f94e410b5409e601b9d30ebe09b"}
Message was sent while issue was closed.
Description was changed from ========== Problem: AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. The DCHECK in ThreadTaskRunnerHandle::Get() causes the crash, and it should. Solution: Since the process of loading rules should always be asynchronous, there should be no check in the RulesLoaded, the later DCHECK would serve the purpose. Therefore, change the unit-test country code from "ZZ" to "OZ", because the data for ZZ can't be parsed, and the success = 0, and since the tests are synchronous, there would be a crash. For "OZ", there is no data, therefore success = 1, but there would be no rules loaded for that, which serves the purpose. ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed, because for the case of AddressValidator::RULES_UNAVAILABLE, success == true, but status != AddressValidator::SUCCESS. Also, since the AddressValidatorTest loads rules synchronously, success should always be true, or there will be a crash. Add an extra check in this unittest, to make sure that the rules are not loaded. Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. BUG=735036 ========== to ========== Problem: AddressValidatorTest.SubKeysNotLoaded would crash, because AddressValidator would not handle the synchronous cases. The DCHECK in ThreadTaskRunnerHandle::Get() causes the crash, and it should. Solution: Since the process of loading rules should always be asynchronous, there should be no check in the RulesLoaded, the later DCHECK would serve the purpose. Therefore, change the unit-test country code from "ZZ" to "OZ", because the data for ZZ can't be parsed, and the success = 0, and since the tests are synchronous, there would be a crash. For "OZ", there is no data, therefore success = 1, but there would be no rules loaded for that, which serves the purpose. ASSERT_EQ(success, status == AddressValidator::SUCCESS) is changed, because for the case of AddressValidator::RULES_UNAVAILABLE, success == true, but status != AddressValidator::SUCCESS. Also, since the AddressValidatorTest loads rules synchronously, success should always be true, or there will be a crash. Add an extra check in this unittest, to make sure that the rules are not loaded. Rename SubKeysNotLoaded to SubKeysNotExists, to be clear that no data exists for this country code. BUG=735036 Review-Url: https://codereview.chromium.org/2950353002 Cr-Commit-Position: refs/heads/master@{#483488} Committed: https://chromium.googlesource.com/chromium/src/+/1ccce34079720f94e410b5409e60... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001) as https://chromium.googlesource.com/chromium/src/+/1ccce34079720f94e410b5409e60... |
