tempesta-tech / tempesta-test

Test suite for Tempesta FW
11 stars 4 forks source link

Tests Frang #265

Closed KonsKo closed 1 year ago

KonsKo commented 2 years ago

I prepared my work pass it to Nadia, because now she is gonna make Frang tests. I upload almost everything I have except tests I was stacked with. Something was made some time ago, something not. I disabled several tests with comment Disabled for PR (them should be fixed). I repeat, I did not fix them, because task was passed to Nadia.

Also, there is some cleaning for multiple_listeners.

@ProshNad, after PR is done, take a look of tests and do everything you want.

The related issues is https://github.com/tempesta-tech/tempesta/issues/673

ProshNad commented 2 years ago

It so happened that I already took a branch kk-test and fixed broken tests and added my new tests. Next time I will do everything in my branch, but this time we agreed to leave it like that and do a review in this branch.

pale-emperor commented 2 years ago

I have run the tests on CI node for several times and get stable errors in the tests listed below:

FAIL: test_h2_host_header_as_ipv6 (t_frang.test_host_required.FrangHostRequiredH2TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 520, in test_h2_host_header_as_ipv6
    self._test_base_scenario(
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 578, in _test_base_scenario
    self.assertEqual(
AssertionError: 0 != 1 : Frang limits warning is not shown

======================================================================
FAIL: test_h2_host_header_mismatch (t_frang.test_host_required.FrangHostRequiredH2TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 504, in test_h2_host_header_mismatch
    self._test_base_scenario(
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 578, in _test_base_scenario
    self.assertEqual(
AssertionError: 0 != 1 : Frang limits warning is not shown

======================================================================
FAIL: test_h2_host_header_missing (t_frang.test_host_required.FrangHostRequiredH2TestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 496, in test_h2_host_header_missing
    self._test_base_scenario(
  File "/home/tempesta/tempesta-test/t_frang/test_host_required.py", line 578, in _test_base_scenario
    self.assertEqual(
AssertionError: 2 != 1 : Frang limits warning is not shown

======================================================================
FAIL: test_tls_connection_rate_on_the_limit (t_frang.test_tls_rate_burst.FrangTlsRateBurstTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tempesta/tempesta-test/t_frang/test_tls_rate_burst.py", line 539, in test_tls_connection_rate_on_the_limit
    self.assertEqual(
AssertionError: 1 != 0 : Expected nums of warnings in `journalctl`: 0, but got 1
RomanBelozerov commented 1 year ago

@nickzaev

multiple_listeners group of test has already been fixed in https://github.com/tempesta-tech/tempesta-test/pull/275, so you just need to rebase your branch onto master, resolving conflicts

Done.

The majority of test files contain some base class and just one class directly inherited from it, on the other hand the tests are being grouped by files (not by classes) and it is very unlikely that, say, there will be another class in t_frang/test_client_body_timeout.py any soon. So what's the point having that inheritance approach all over the place?

If we actually fall for that approach of using inheritance and all, the parts of starting off backends and Tempesta should be in setUp method of some base class and not get repeated for every test as it is for now.

All tests are parameterized and general logic is moved to base class. Unnecessary inheritance removed.

About a solid third of the tests code consists of almost identical Nginx configurations which either don't differ at all or just in very minor parts, so it looks more that we test Nginx rather than Tempesta :) Why not just reuse FrangTestCase everywhere possible at least just for consistency's sake?

Constants are massively overused, indeed, what's point of having such constants as ONE?

Nginx replaced to deproxy. Duplicate config and single constants removed.

RomanBelozerov commented 1 year ago

To sum up:

This PR requires changes #360