|Submitter||Diff||Changes||Open Issues||Last Updated|
|Error loading review requests:|
59 bytes, text/x-review-board-request
|Details | Review|
I was trying to land bug 1352889, one of whose effects is to change the hashing used in PLDHashTable. This changes hashtable enumeration order. This change led to a single test failure, in security/manager/ssl/tests/unit/test_cert_blocklist.js . This test failure is easier to understand if I change kTruncateLength from 128 to 4096 here: https://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/testing/modules/Assert.jsm#54 The failure (with the larger kTruncateLength) is: 0:02.25 TEST_STATUS: Thread-1 FAIL revocations.txt should be as expected - "# Auto generated contents. Do not edit.\\nMCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5\\n\\tVCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=\\nMBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E=\\n exJUIJpq50jgqOwQluhVrAzTF74=\\nMBIxEDAOBgNVBAMMB1Rlc3QgQ0E=\\n BVio/iQ21GCi2iUven8oJ/gae74=\\nYW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy\\n YW5vdGhlciBzZXJpYWwu\\n c2VyaWFsMi4=" == "# Auto generated contents. Do not edit.\\nMCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5\\n\\tVCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=\\nMBIxEDAOBgNVBAMMB1Rlc3QgQ0E=\\n BVio/iQ21GCi2iUven8oJ/gae74=\\nMBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E=\\n exJUIJpq50jgqOwQluhVrAzTF74=\\nYW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy\\n YW5vdGhlciBzZXJpYWwu\\n c2VyaWFsMi4=" /home/dbaron/builds/mozilla-central/obj/firefox-debugopt/_tests/xpcshell/security/manager/ssl/tests/unit/test_cert_blocklist.js:check_revocations_txt_contents:208 /home/dbaron/builds/mozilla-central/obj/firefox-debugopt/_tests/xpcshell/security/manager/ssl/tests/unit/test_cert_blocklist.js:run_test/<:303 /home/dbaron/builds/mozilla-central/mozilla/testing/xpcshell/head.js:_run_next_test:1569 /home/dbaron/builds/mozilla-central/mozilla/testing/xpcshell/head.js:run:703 /home/dbaron/builds/mozilla-central/mozilla/testing/xpcshell/head.js:_do_main:211 /home/dbaron/builds/mozilla-central/mozilla/testing/xpcshell/head.js:_execute_test:546 -e:null:1 0:02.25 LOG: Thread-1 INFO exiting test 0:02.25 LOG: Thread-1 ERROR 2147500036 This shows the lines in the expected output being reordered. What's the best way to proceed here? Is it actually important that the hashtable enumeration order remain unchanged, or does the test just happen to depend on it? If so, is there a better way to fix the test than to just change the test expectation? (Note that I may need to do this *again* for bug 1352890.)
There is some structure to the file that we're testing there, but we can ensure that it is as expected without depending on the hashtable enumeration order. As far as I understand, these are the requirements we need to ensure: 1. The file starts with "# Auto generated contents. Do not edit." 2. The file has four sets of related lines: MCIxIDAeBgNVBAMMF0Fub3RoZXIgVGVzdCBFbmQtZW50aXR5 \tVCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8= MBIxEDAOBgNVBAMMB1Rlc3QgQ0E= BVio/iQ21GCi2iUven8oJ/gae74= MBgxFjAUBgNVBAMMDU90aGVyIHRlc3QgQ0E= exJUIJpq50jgqOwQluhVrAzTF74= YW5vdGhlciBpbWFnaW5hcnkgaXNzdWVy YW5vdGhlciBzZXJpYWwu c2VyaWFsMi4= where these sets may appear in any order, but the contents of each set should remain together (that is, two sets shouldn't be interleaved). For the last set in this list (the one that has two indented lines after the main line), the order of the indented lines shouldn't matter either (but to be clear, the heading line of each set should always be first). 3. The file shouldn't contain anything else. Let me know if I should elaborate on anything here. If that's enough detail to fix this test with respect to bug 1352889 and if you have the time, feel free to write a patch - I can review it. If not, I'll find someone to address this as soon as I can.
Er, the lint opt source-test-mozlint-eslint test actually caught something, so revised patch coming...
Comment on attachment 8864344 [details] Bug 1353458 - Make test_cert_blocklist more flexible about order of lines in revocations.txt. https://reviewboard.mozilla.org/r/135990/#review139284 Nice - I like this solution. Thank you for taking the time to do this.
Attachment #8864344 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4225b0990255df9f75157a74c249878a2f47103 Bug 1353458 - Make test_cert_blocklist more flexible about order of lines in revocations.txt. r=keeler
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.