[
https://issues.apache.org/jira/browse/TS-3782?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14650467#comment-14650467
]
Thomas Jackson commented on TS-3782:
------------------------------------
I generally prefer reviewing code on github since I can comment there ;) but
I'll try and do it here in the comments:
h1. normal_scenario_tests_001.patch
{code}
+try:
+ import hyper
+except Exception as e:
+ helpers.unittest.SkipTest('Cannot import hyper, skipping tests for HTTP/2')
{code}
This will be ImportError, no need to catch all exceptions :)
{code}
+ # HTTP2 configs
+ cls.configs['records.config']['CONFIG']['proxy.config.http2.enabled']
= 1
+
cls.configs['records.config']['CONFIG']['proxy.config.http.server_ports'] += '
{0}:ssl'.format(cls.http2_port)
+
cls.configs['records.config']['CONFIG']['proxy.config.ssl.server.cert.path'] =
helpers.tests_file_path('rsa_keys')
+
+ cls.configs['records.config']['CONFIG'].update({
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'http2.*|ssl.*'
+ })
{code}
Style thing-- you modify this dict two ways, I personally prefer the second--
but I'd just pick one for the test to be consistent :)
{code}
+ def hello(request):
+ return 'hello'
+
+ cls.http_endpoint.add_handler('/', hello)
{code}
If all you need is a basic endpoint I'd suggest using
[HTTPBinCase|https://github.com/apache/trafficserver-qa/blob/master/tsqa/test_cases.py#L159]
{code}
+ def __traffic_out(self):
+ '''
+ Cat traffic.out
+ '''
+ traffic_out_path = os.path.join(self.environment.layout.logdir,
'traffic.out')
+ traffic_out = open(traffic_out_path)
+ for line in traffic_out.read
{code}
I see that you use this to print out on a test failure, that *might* be a lot
of output so you might want to put that on the debug level-- and I might
actually make that the default for debug-- seems like a nice-ish way to avoid
going to the temp directory ;) Keep in mind though that some messages go to
diags.log as well.
h1. exceptional_scenario_tests_001.patch
{code}
# Run all tests.
-test: $(VIRTUALENV_DIR)
+test: $(VIRTUALENV_DIR) setup_h2spec
@. $(VIRTUALENV_DIR)/bin/activate && $(VIRTUALENV_DIR)/bin/nosetests
--with-xunit -sv --logging-level=INFO
{code}
I'm not sure if we want to put this up at the top like this-- my concern would
be that we'd require this h2spec to run *any* tests. Maybe this would be better
done similarly how you do your conditional import within the test? Opinions
[~zwoop]?
{code}
+ def test_http2_spec_section(self):
+ '''
+ Test HTTP/2 w/ h2spec (Exceptional Scenario)
+ '''
+ # TODO Use all test cases after ATS can pass them
+ # sections = ['3.5', '4.2', '4.3', '5.1', '5.3.1', '5.4.1', '5.5',
+ # '6.1', '6.2', '6.3', '6.4', '6.5', '6.5.2', '6.7', '6.8',
+ # '6.9', '6.9.1', '6.10', '8.1', '8.1.2', '8.1.2.2',
'8.1.2.3', '8.1.2.6', '8.2']
+ sections = ['3.5', '4.2', '4.3', '5.3.1',
+ '6.2', '6.3', '6.5.2', '6.7', '6.8',
+ '6.9', '8.1.2', '8.1.2.3', '8.2']
+
+ for section in sections:
+ self.assertEquals(0, self.__callH2Spec(section))
+ self.assertIn('All tests passed', self.stdout)
{code}
for the assertIn line you might want to set msg='Section X passed' otherwise if
this doesn't pass it'll be hard to know which section failed :)
In addition, You might want to run the other tests and mark them as expected
failures-- just so we know when we fix them.
Overall, looks good, few minor changes to be made then we can merge this.
Thanks for the tests!
> Add tests for HTTP/2 in TSQA
> ----------------------------
>
> Key: TS-3782
> URL: https://issues.apache.org/jira/browse/TS-3782
> Project: Traffic Server
> Issue Type: Test
> Components: CI, HTTP/2
> Reporter: Masaori Koshiba
> Assignee: Thomas Jackson
> Labels: review
> Fix For: 6.0.0
>
> Attachments: exceptional_scenario_tests_001.patch,
> normal_scenario_tests_001.patch
>
>
> Add tests for HTTP/2 in TSQA. IMO, it is better to add two types of tests for
> HTTP/2 below.
> 1. Normal Scenario Tests
> - Do HTTP requests with [hyper|https://github.com/lukasa/hyper] or other
> HTTP/2 client.
> 2. Exceptional Scenario Tests
> - [h2spec|https://github.com/summerwind/h2spec] looks good test tool for
> edge cases of HTTP/2.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
