New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 767108 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Eliminate transport_security_state_static_unittest0.json

Project Member Reported by lgar...@chromium.org, Sep 20 2017

Issue description

transport_security_state_static_unittest0.json is a partial copy of the preload list used for Cronet tests. Some of those tests explicitly run sanity-check tests on actual entries in the preload list, which... aren't preloaded on Cronet.

This causes extra work (you have to remember to update the cronet test file if you happen to touch a relevant test), and – since we don't have Cronet bots yet – extra pain: https://chromium-review.googlesource.com/c/chromium/src/+/673883

We should either:
- Use the real preload list for unit tests on Cronet.
- Disable corresponding tests on Cronet (may require splitting up tests).

xunjieli@ prefers the latter.

[1] https://cs.chromium.org/chromium/src/net/http/transport_security_state_static_unittest0.json
 
My preference is to separate transport security tests into two categories (1) general functionality tests (2) tests that asserts on correctness of the real preload list. 

For (1), those tests shouldn't rely on the real preload list, but on transport_security_state_static_unittest0.json.
For (2), we disable these tests if INCLUDE_TRANSPORT_SECURITY_STATE_PRELOAD_LIST is undefined.

That sounds fine.

Would you have any time to do that? :-)
No. Unfortunately, I am swamped with things that I need to do. 
I am not the best person to do this though -- my understanding of Transport Security concepts is really minimal :)
Owner: mart...@martijnc.be
Status: Started (was: Available)
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b1383dad418304962de75e402ea25d645d02092c

commit b1383dad418304962de75e402ea25d645d02092c
Author: Martijn Croonen <martijn@martijnc.be>
Date: Wed Oct 11 11:56:35 2017

Split up transport security state related tests.

All transport security state tests currently depend on the real preload
list. This has caused issues when the preload list is updated. This CL
splits up the tests in two categories:

  1: tests that test functionality related to the static transport
     security state implementation.
  2: tests that assert the correctness of the real preload list.

The tests that fall under (1) no longer depend on entries in the real
preload list but use a test-only preload list. The entries in (2) use
the real preload list but are disabled when the preload list is not
used.

Bug:  767108 
Change-Id: I71ad58670d0e2366d76602b5a6d799e3c48269a4
Reviewed-on: https://chromium-review.googlesource.com/706786
Reviewed-by: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Martijn Croonen <martijnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507954}
[modify] https://crrev.com/b1383dad418304962de75e402ea25d645d02092c/net/BUILD.gn
[modify] https://crrev.com/b1383dad418304962de75e402ea25d645d02092c/net/http/BUILD.gn
[modify] https://crrev.com/b1383dad418304962de75e402ea25d645d02092c/net/http/http_security_headers_unittest.cc
[delete] https://crrev.com/be40400e2f2c19fea7aed40e64d470f0dd5814af/net/http/transport_security_state_static_unittest0.json
[add] https://crrev.com/b1383dad418304962de75e402ea25d645d02092c/net/http/transport_security_state_static_unittest_default.json
[add] https://crrev.com/b1383dad418304962de75e402ea25d645d02092c/net/http/transport_security_state_static_unittest_default.pins
[modify] https://crrev.com/b1383dad418304962de75e402ea25d645d02092c/net/http/transport_security_state_unittest.cc
[modify] https://crrev.com/b1383dad418304962de75e402ea25d645d02092c/net/tools/transport_security_state_generator/README.md
[modify] https://crrev.com/b1383dad418304962de75e402ea25d645d02092c/net/url_request/url_request_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment