UnitTests using assertTrue(x, y) instead of assertEqual(x, y)

[baolsen]

Describe the bug

Some unit tests, such as those in test_apigw.py, use assertions like these:

self.assertTrue(len(resources), 1)

These will resolve even when len(resources) is greater than one. Instead, either of the following should be used:

self.assertTrue(len(resources) == 1) self.assertEqual(len(resources), 1)

Arguably the second form is better as it will show more meaningful error information when the test fails.

There should be some check to prevent using assertTrue with multiple arguments in the codebase to avoid this error in future.

This happens because the function signature of assertTrue accepts 2 arguments:

assertTrue(self, expr, msg=None)

In other words, the following assertion will pass: self.assertTrue(1==1, 3) And the one below will fail with the message "3" self.assertTrue(1==2, 3)

What did you expect to happen?

Assertion should fail when 2 != 1 but it doesn't

Cloud Provider

Amazon Web Services (AWS)

Cloud Custodian version and dependency information

Latest code from github

Policy

No response

Relevant log/traceback output

No response

Extra information or context

No response

[kapilt]

good catch, yeah we should probably excise all of those from tests to use assert equal .. note since we're using pytest we could also just assert x == y albeit we mostly do that in non unittest case derived tests.

[kapilt]

there's quite a few of these

./tests/test_webhook.py:        self.assertTrue(self.load_policy(data=policy, validate=True))
./tests/test_webhook.py:        self.assertTrue(self.load_policy(data=policy, validate=True))
./tests/test_sagemaker.py:        self.assertTrue(tags[0]["Key"], "custodian_cleanup")
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(notebook["NotebookInstanceStatus"], "Pending")
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(notebook["NotebookInstanceStatus"], "Stopping")
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(notebook["NotebookInstanceStatus"], "Deleting")
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(tags[0], "custodian_cleanup")
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(tags[0]["Key"], "required-tag")
./tests/test_sagemaker.py:        self.assertTrue(tags[0]["Key"], "required-value")
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(tags[0], "custodian_cleanup")
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_sagemaker.py:        self.assertTrue(tags[0], "custodian_cleanup")
./tests/test_sagemaker.py:        self.assertTrue(len(resources), 1)
./tests/test_aws.py:        self.assertTrue(isinstance(sink, aws.MetricsOutput))
./tests/test_kinesis.py:        self.assertTrue(len(resources), 1)
./tests/test_sfn.py:        self.assertTrue(len(resources), 1)
./tests/test_sfn.py:        self.assertTrue(resources[0]['name'], 'test')
./tests/test_backup.py:        self.assertTrue(len(resources), 1)
./tests/test_mq.py:        self.assertTrue(len(resources), 1)
./tests/test_mq.py:        self.assertTrue(len(resources), 1)
./tests/test_mq.py:        self.assertTrue(len(resources), 1)
./tests/test_s3.py:        self.assertTrue(info["ContentLength"], post_info["ContentLength"])
./tests/test_policy.py:        self.assertTrue(isinstance(p.load_resource_manager(), EC2))
./tests/test_manager.py:        self.assertTrue(isinstance(ec2.actions[0], Tag))
./tests/test_manager.py:        self.assertTrue(isinstance(ec2.actions[0], Tag))
./tests/test_notify.py:        self.assertTrue(self.load_policy(policy, validate=True))
./tests/test_apigw.py:        self.assertTrue(len(resources), 1)
./tests/test_apigw.py:        self.assertTrue(len(resources), 1)
./tests/test_apigw.py:        self.assertTrue(resources[0]['id'], 'am0c2fyskg')
./tests/test_apigw.py:        self.assertTrue(len(resources), 1)
./tests/test_cli.py:        self.assertTrue("i-014296505597bf519", json.loads(output)[0]['InstanceId'])
./tests/test_ec2.py:        self.assertTrue(len(resp['Reservations'][0]['Instances']), 1)
./tests/test_lambda.py:        self.assertTrue(len(resources), 1)
./tests/test_filters.py:        self.assertTrue(ef.process([instance()], event))
./tests/test_query.py:        self.assertTrue("Using cached internet-gateway: 3", output.getvalue())
./tests/test_sns.py:        self.assertTrue(len(resources), 1)
./tests/test_sns.py:        self.assertTrue(attributes['Attributes']['KmsMasterKeyId'], 'alias/aws/sns')
./tests/test_sns.py:        self.assertTrue(tags[0]["Key"], "custodian_cleanup")
./tests/test_ecs.py:        self.assertTrue(resources[0]['serviceName'], 'test-yes-tag')
./tests/test_mu.py:        self.assertTrue(len(functions), 1)
./tests/test_mu.py:        self.assertTrue(get("foo"), "foo.py")
./tests/test_mu.py:            self.assertTrue(get("bar"), "bar.pyc")
./tests/test_kms.py:        self.assertTrue(len(resources), 1)
./tests/test_ssm.py:            self.assertTrue(e, client.exceptions.InvalidDocument)
./tests/test_ssm.py:            self.assertTrue(e, client.exceptions.InvalidDocumentOperation)
./tests/test_schema.py:        self.assertTrue(isinstance(result[0], ValueError))
./tests/test_schema.py:        self.assertTrue("'asdf' is not of type 'boolean'" in str(err).replace("u'", "'"))
./tests/test_glacier.py:        self.assertTrue("RemoveMe" not in [s["Sid"] for s in data.get("Statement", ())])
./tests/test_kafka.py:        self.assertTrue(len(resources), 1)
./tests/test_rds.py:        self.assertTrue(len(resources), 1)
./tests/test_rds.py:        self.assertTrue(len(resources), 1)
./tests/test_workspaces.py:        self.assertTrue(len(resources), 1)
./tests/test_elasticsearch.py:        self.assertTrue(len(resources), 1)
./tests/test_cwl.py:        self.assertTrue(len(resources), 1)
./tests/test_fsx.py:        self.assertTrue(len(resources), 1)
./tests/test_fsx.py:        self.assertTrue(len(fs), 1)
./tests/test_fsx.py:        self.assertTrue(len(resources), 1)
./tests/test_fsx.py:        self.assertTrue(len(fs), 1)
./tests/test_fsx.py:        self.assertTrue(len(resources), 1)
./tests/test_fsx.py:        self.assertTrue(len(fs), 1)
./tests/test_fsx.py:        self.assertTrue(len(resources), 1)
./tests/test_fsx.py:                self.assertTrue(len(b['Tags']), 1)
./tests/test_fsx.py:        self.assertTrue(len(resources), 1)
./tests/test_fsx.py:                self.assertTrue(len(b['Tags']), 1)
./tests/test_fsx.py:        self.assertTrue(len(resources), 1)
./tests/test_airflow.py:        self.assertTrue(len(resources), 1)
./tools/c7n_kube/tests/test_actions.py:        self.assertTrue(len(resources), 1)
./tools/c7n_kube/tests/test_custom_resource.py:        self.assertTrue(len(resources), 1)
./tools/c7n_kube/tests/test_custom_resource.py:        self.assertTrue(len(resources), 1)
./tools/c7n_azure/tests_azure/tests_resources/test_keyvault_keys.py:        self.assertTrue(resources[0]['c7n:kty'].lower(), 'rsa')
./tools/c7n_azure/tests_azure/tests_resources/test_cost_management_export.py:        self.assertTrue(self._get_policy(filters=[{'type': 'last-execution', 'age': 30}],

[ajkerrigan]

Thanks for the report - closed via #7914