search

scylladb / python-driver-matrix

3 stars 11 forks source link

refactor: Refactoring all project #31

Closed Orenef11 closed 2 years ago

Orenef11 commented 2 years ago
Orenef11 commented 2 years ago

@roydahan Please review this PR by reviewing each commit separately Also, this PR fixes scylladb#4

roydahan commented 2 years ago

@fruch / @dkropachev can you please help with review?

roydahan commented 2 years ago

@Orenef11 is it ready for review? If so, please arrange your commits and move it from draft to "ready for review".

Orenef11 commented 2 years ago

@Orenef11 is it ready for review? If so, please arrange your commits and move it from draft to "ready for review".

I'm waiting until the JOB will finish to update the YAML files with the names of the failed tests.

@dkropachev I'll send you a message when the PR will be ready.

Orenef11 commented 2 years ago

@fruch @roydahan @dkropachev The PR is ready for review. I checked it and it working well. The run with protocol 4 is failed because of cassandra.custom_query_handler_class=org.apache.cassandra.cql3.CustomPayloadMirroringQueryHandler key is missing. Full output:

Scylla version 4.7.dev-0.20220103.8774fc83d with build-id 42b67b0cfeb5f80ba7e03c1f4f86fa3b16c1d315 starting ...
command used: "/home/oren/Desktop/github/datastax-python-driver/tests/integration/ccm/test_cluster/node1/bin/scylla --options-file /home/oren/Desktop/github/datastax-python-driver/tests/integration/ccm/test_cluster/node1/conf/scylla.yaml --log-to-stdout 1  -Dcassandra.custom_query_handler_class=org.apache.cassandra.cql3.CustomPayloadMirroringQueryHandler --api-address 127.0.0.1 --collectd-hostname oren-Latitude-7400.node1 --developer-mode true --smp 1 --memory 512M --default-log-level info --collectd 0 --overprovisioned --prometheus-address 127.0.0.1 --unsafe-bypass-fsync 1 --kernel-page-cache 1 --max-networking-io-control-blocks 1000"
fruch commented 2 years ago

@fruch @roydahan @dkropachev The PR is ready for review. I checked it and it working well. The run with protocol 4 is failed because of cassandra.custom_query_handler_class=org.apache.cassandra.cql3.CustomPayloadMirroringQueryHandler key is missing. Full output:

Scylla version 4.7.dev-0.20220103.8774fc83d with build-id 42b67b0cfeb5f80ba7e03c1f4f86fa3b16c1d315 starting ...
command used: "/home/oren/Desktop/github/datastax-python-driver/tests/integration/ccm/test_cluster/node1/bin/scylla --options-file /home/oren/Desktop/github/datastax-python-driver/tests/integration/ccm/test_cluster/node1/conf/scylla.yaml --log-to-stdout 1  -Dcassandra.custom_query_handler_class=org.apache.cassandra.cql3.CustomPayloadMirroringQueryHandler --api-address 127.0.0.1 --collectd-hostname oren-Latitude-7400.node1 --developer-mode true --smp 1 --memory 512M --default-log-level info --collectd 0 --overprovisioned --prometheus-address 127.0.0.1 --unsafe-bypass-fsync 1 --kernel-page-cache 1 --max-networking-io-control-blocks 1000"

see https://github.com/scylladb/python-driver/commit/3ea7636158383633fb490724099332ceb1498249#diff-6ec5224a3ecfa5b56e1dce12620fffa868d0b50190add628982d3596085b7e9d

You'll need to amend the patch in a similar way, and skip those jvm_args cause scylla doesn't support it. it was failing on datastax version, right ?

Also @Orenef11 seems like there are lots of other failures, aren't you have an update version of the test skip yaml ?

fruch commented 2 years ago

@Orenef11 On one hand:

04:21:48  INFO:root:total_tests: 385, errors: 0, failures: 1, skipped: 97, xpassed: 0, xfailed: 0, passed: 249, ignored_in_analysis: 38

On the other hand all ignored_in_analysis are shown as failure here: https://jenkins.scylladb.com/view/staging/job/scylla-staging/job/Oren/job/python-driver-matrix-test1/14/testReport/

So or we stop reporting to jenkins, and create a different report, or modify the xml report and remove those ignored, or at least change them to skip

I'm not sure why not deselect them as it was before. it make it much more easy. also globbing might be a nice addition so you can easily select a file or a whole class of tests. (I know in java matrix, I was using it)

Orenef11 commented 2 years ago

So or we stop reporting to jenkins, and create a different report, or modify the xml report and remove those ignored, or at least change them to skip

I'm not sure why not deselect them as it was before. it make it much more easy. also globbing might be a nice addition so you can easily select a file or a whole class of tests. (I know in java matrix, I was using it)

Yes, you're right, for protocol 4 I don't add the "skip" tests because of the jvm_args issue. I'll fix run it and I'll fill the "skip" tests.

@Orenef11 On one hand:

04:21:48  INFO:root:total_tests: 385, errors: 0, failures: 1, skipped: 97, xpassed: 0, xfailed: 0, passed: 249, ignored_in_analysis: 38

On the other hand all ignored_in_analysis are shown as failure here: https://jenkins.scylladb.com/view/staging/job/scylla-staging/job/Oren/job/python-driver-matrix-test1/14/testReport/

So or we stop reporting to jenkins, and create a different report, or modify the xml report and remove those ignored, or at least change them to skip

I'm not sure why not deselect them as it was before. it make it much more easy. also globbing might be a nice addition so you can easily select a file or a whole class of tests. (I know in java matrix, I was using it)

I want the report to be detailed and contain a lot more information in case a test fails. Therefore, I run all the tests to check if a test marked "skip" passes or not in the Scylla new version (every job runs on the latest Scylla version).

if the user wants to run one file/one test/one class, he can do it like that:

fruch commented 2 years ago

I want the report to be detailed and contain a lot more information in case a test fails. Therefore, I run all the tests to check if a test marked "skip" passes or not in the Scylla new version (every job runs on the latest Scylla version).

if the user wants to run one file/one test/one class, he can do it like that:

  • Specific file: tests/integration/standard/test_authentication.py or tests.integration.standard.test_authentication.py:
  • Specific class name: tests/integration/standard/test_authentication.py::SaslAuthenticatorTests or tests.integration.standard.test_authentication.py::SaslAuthenticatorTests
  • Specific test name: tests/integration/standard/test_authentication.py::SaslAuthenticatorTests::test_host_passthrough or tests.integration.standard.test_authentication.py::SaslAuthenticatorTests::test_host_passthrough

I didn't understand how running a single test has todo with it. the question is where people would be looking at, if that on Jenkins, we need to make sure the result reflect the current situation, and not some specific log that show we have one failure.

If there a different way to show the results clearly, at least attach it as an artifact to the job.

Right now the outcome looks like there are lots of tests failing even that this are marked to be skipped. kind of futile.

roydahan commented 2 years ago

@Orenef11 please let me know when it's ready for review.

Orenef11 commented 2 years ago

@Orenef11 please let me know when it's ready for review.

@roydahan You can review it. I running this run (https://jenkins.scylladb.com/view/staging/job/scylla-staging/job/Oren/job/python-driver-matrix-test1/22/) to add all flaky tests.

fruch commented 2 years ago

@Orenef11, results looks much better, but the job timed out.

Also can you explain what is the "flaky" section in the ignore yaml ? what exactly is the flow for those tests. we run them or not, we care for their outcome of not ?

Orenef11 commented 2 years ago

Also can you explain what is the "flaky" section in the ignore yaml ? what exactly is the flow for those tests. we run them or not, we care for their outcome of not ?

Thanks. I was changed the job's timeout from 7 to 10 hours but job#22 failed after 7 hours.

The new logic runs all the tests and only at the end of the run analyzes the result. Because of this change, I added the following values to the report:

I created a pytest file to test the report logic.

I can change the logic and not run those tests.

Orenef11 commented 2 years ago

@fruch @roydahan Most of the tests that are under the "flaky" section are tests that verify the number of warning messages.

 <testcase classname="tests.integration.standard.test_authentication_misconfiguration.MisconfiguredAuthenticationTests" name="test_connect_no_auth_provider" time="229.775">
      <failure message="AssertionError: 0 != 1">self = &lt;tests.integration.standard.test_authentication_misconfiguration.MisconfiguredAuthenticationTests testMethod=test_connect_no_auth_provider&gt;

    def test_connect_no_auth_provider(self):
        cluster = TestCluster()
        cluster.connect()
        cluster.refresh_nodes()
        down_hosts = [host for host in cluster.metadata.all_hosts() if not host.is_up]
&gt;       self.assertEqual(len(down_hosts), 1)
E       AssertionError: 0 != 1

tests/integration/standard/test_authentication_misconfiguration.py:46: AssertionError</failure>
    </testcase>
<testcase classname="tests.integration.standard.test_cluster.DeprecationWarningTest" name="test_deprecation_warnings_meta_refreshed" time="0.003">
      <failure message="AssertionError: 3 != 1">self = &lt;tests.integration.standard.test_cluster.DeprecationWarningTest testMethod=test_deprecation_warnings_meta_refreshed&gt;

    def test_deprecation_warnings_meta_refreshed(self):
        &quot;&quot;&quot;
        Tests the deprecation warning has been added when enabling
        metadata refreshment

        @since 3.13
        @jira_ticket PYTHON-890
        @expected_result the deprecation warning is emitted

        @test_category logs
        &quot;&quot;&quot;
        with warnings.catch_warnings(record=True) as w:
            cluster = TestCluster()
            cluster.set_meta_refresh_enabled(True)
&gt;           self.assertEqual(len(w), 1)
E           AssertionError: 3 != 1

tests/integration/standard/test_cluster.py:1560: AssertionError</failure>
    </testcase>
fruch commented 2 years ago

Most of the tests that are under the "flaky" section are tests that verify the number of warning messages.

that doesn't answer the question what flaky mean in our context ? why not put those test in ignore ? i.e. they are not going to fix themselves magically, how they are different from any other test we ignore.

Orenef11 commented 2 years ago

Most of the tests that are under the "flaky" section are tests that verify the number of warning messages.

that doesn't answer the question what flaky mean in our context ? why not put those test in ignore ? i.e. they are not going to fix themselves magically, how they are different from any other test we ignore.

The goal is for the YAML file to be empty of test names. There is a big difference between a flaky test and a failed test. A flaky test fails from time to time unlike a test marked as "ignore" that failed every time. A "ignore" test will be mark as a pass only if the problem was solved or the logic of the test is changed. Therefore, a test marked as ignore and passed in run X, will be marked as xpass (the run is marked as failed) and the user needs to remove it from the YAML file.

fruch commented 2 years ago

Most of the tests that are under the "flaky" section are tests that verify the number of warning messages.

that doesn't answer the question what flaky mean in our context ? why not put those test in ignore ? i.e. they are not going to fix themselves magically, how they are different from any other test we ignore.

The goal is for the YAML file to be empty of test names. There is a big difference between a flaky test and a failed test. A flaky test fails from time to time unlike a test marked as "ignore" that failed every time. A "ignore" test will be mark as a pass only if the problem was solved or the logic of the test is changed. Therefore, a test marked as ignore and passed in run X, will be marked as xpass (the run is marked as failed) and the user needs to remove it from the YAML file.

I'm just arguing that the process for ignored and flaky is the same, you mark them and skip them until fixed.

Orenef11 commented 2 years ago

Most of the tests that are under the "flaky" section are tests that verify the number of warning messages.

that doesn't answer the question what flaky mean in our context ? why not put those test in ignore ? i.e. they are not going to fix themselves magically, how they are different from any other test we ignore.

The goal is for the YAML file to be empty of test names. There is a big difference between a flaky test and a failed test. A flaky test fails from time to time unlike a test marked as "ignore" that failed every time. A "ignore" test will be mark as a pass only if the problem was solved or the logic of the test is changed. Therefore, a test marked as ignore and passed in run X, will be marked as xpass (the run is marked as failed) and the user needs to remove it from the YAML file.

I'm just arguing that the process for ignored and flaky is the same, you mark them and skip them until fixed.

No, because a test that is "flaky" has to verify it's passed in 3-4 runs, unlike a "ignore" test that can be removed from the YAML file after one run.

roydahan commented 2 years ago

As we discussed on a call, change the "ignore" behaviour that even if it passes, it won't fail the test.