[RHINENG-1284] Tests are failing in test_api_assignment_rules_create.py #1440

jpramos123 · 2023-07-28T21:11:29Z

Overview

This PR is being created to address RHINENG-1284.

The issue was in the Model, that was not creating the correct constrains.
Pytest uses the Models directly, and do not run the migration scrips to create the tables.

PR Checklist

Keep PR title short, ideally under 72 characters
Descriptive comments provided in complex code blocks
Tests: validate optimal/expected output
Tests: validate exceptions and failure scenarios
Tests: edge cases
Recovers or fails gracefully during potential resource outages (e.g. DB, Kafka)
Uses type hinting, if convenient
Documentation, if this PR changes the way other services interact with host inventory
Links to related PRs

Secure Coding Practices Documentation Reference

You can find documentation on this checklist here.

Secure Coding Checklist

tests/test_api_assignment_rules_create.py

kruai · 2023-08-01T13:03:42Z

tests/fixtures/db_fixtures.py

+    def _db_get_assignment_rule(ar_id, group_id):
+        return AssignmentRule.query.get((ar_id, group_id))


Why is this necessary? Isn't the assignment rule ID unique?

It led to a issue with multiple primary keys, and was not able to fetch the data using only the ID.
See the error below:

sqlalchemy.exc.InvalidRequestError: Incorrect number of values in identifier to formulate primary key for query.get(); primary key columns are 'assignment_rules.id','assignment_rules.group_id'

Ah... so then my follow-up question is, why do we need the primary key to consist of both the ID and group ID, when the ID should already be unique and non-nullable?
If we make the primary key the combination of those two columns, we technically open up the possibility to having duplicate values for ID (as long as group_id is different)

We do not hehe.

But it was initially created this way:
https://github.com/RedHatInsights/insights-host-inventory/pull/1377/files#diff-6b8bcd51a947a9eb1db19cd9157a27b5bea41a8191cca8ad7b2ee297b0a3e9f7R28

In this case, we can create a new migration to remove the constraint from this column. As we are already making sure group_id is unique: https://github.com/RedHatInsights/insights-host-inventory/pull/1418/files#diff-be43a8e7819671e5eca91faed55b391bf96c0a0149d54a213c8fe3bf3163447a

Thoughts?

We have an idxassrulesorgid which should work to make org_id unique, but maybe what we needed was a unique constraint.

insights-host-inventory/migrations/versions/1689cda89252_add_assignment_rules_table.py

Line 38 in 85e95ce

op.create_index("idxassrulesorgid", "assignment_rules", ["org_id"])

Personally, I don't think it makes sense for the table's primary key to be a combination of unique IDs. If we already have an id field, that should be the primary key; otherwise it adds unnecessary complexity because you need both the assignment rule ID and its group ID in order to fetch the record, right?

tests/test_api_assignment_rules_create.py

kruai · 2023-08-01T13:13:31Z

tests/test_api_assignment_rules_get.py

@@ -38,10 +38,10 @@ def test_get_assignment_rule_by_name(db_create_assignment_rule, db_create_group,


 def test_get_assignment_rule_with_bad_name(db_create_assignment_rule, db_create_group, api_get):


Not an issue with your changes, but am I missing the point of this test? It doesn't appear to be testing any scenario related to having a bad name (and both responses return 200)

In this case, it validates that 3 rules were created, make an api call with a name that will not be in the list, and validate the return is an empty list.

We do not trigger an error when fetching rules that are not in the DB

@thearifismail Can you add more context here?

Thanks

from what I read, the query.filter function doesn't return an error if no results were found

Co-authored-by: Muhammad Arif <[email protected]>

Co-authored-by: Asa Price <[email protected]>

FabriciaDinizRH · 2023-08-01T16:24:48Z

/retest

FabriciaDinizRH · 2023-08-02T13:00:06Z

app/models.py

@@ -524,7 +528,7 @@ def update(self, input_ar):
    account = db.Column(db.String(10))
    name = db.Column(db.String(255), nullable=False)
    description = db.Column(db.String(255))
-    group_id = db.Column(UUID(as_uuid=True), ForeignKey("groups.id"))
+    group_id = db.Column(UUID(as_uuid=True), ForeignKey("groups.id"), primary_key=True)


didn't this got removed because it broke the filtering?
https://redhat-internal.slack.com/archives/CQFKM031T/p1689276193983449?thread_ts=1689271922.019339&cid=CQFKM031T

We can remove it from the model.py file and create a new migration to remove this constraint from the column, or we can add it as this PR suggests and it will be aligned with the DB modeling we have today.

@jpramos123 I was checking the migration files and since this change doesn't break the tests we can leave it as it is.

FabriciaDinizRH · 2023-08-02T14:13:47Z

tests/test_api_assignment_rules_get.py

@@ -38,10 +38,10 @@ def test_get_assignment_rule_by_name(db_create_assignment_rule, db_create_group,


 def test_get_assignment_rule_with_bad_name(db_create_assignment_rule, db_create_group, api_get):


from what I read, the query.filter function doesn't return an error if no results were found

FabriciaDinizRH · 2023-08-02T14:27:45Z

tests/fixtures/db_fixtures.py

+    def _db_get_assignment_rule(ar_id, group_id):
+        return AssignmentRule.query.get((ar_id, group_id))


We have an idxassrulesorgid which should work to make org_id unique, but maybe what we needed was a unique constraint.

insights-host-inventory/migrations/versions/1689cda89252_add_assignment_rules_table.py

Line 38 in 85e95ce

op.create_index("idxassrulesorgid", "assignment_rules", ["org_id"])

jpramos123 · 2023-08-02T17:55:23Z

@FabriciaDinizRH Indexes are used to enhance queries performances, making the data easily available!
See: https://www.postgresql.org/docs/current/indexes.html

roliveri · 2024-06-20T20:59:31Z

I have a question, but it's not specific to the issue addressed by this PR. It's more specific to the test scenario that uncovered this issue.

In the ticket, it says the problem can be reproduced by running 2 separate test cases in the given order:

1. pytest tests/test_api_assignment_rules_create.py::test_create_assignemnt_rule_same_name
2. pytest tests/test_api_assignment_rules_create.py::test_create_assignemnt_rule_same_group

What concerns me here is the fact that not only can the artifacts of running a test case affect the subsequent test cases, it can also have such impact across separate pytest runs. I always had the impression that each test case should be insulated from the others. In fact, in other projects I worked on, we used to randomize the test run order to ensure that was the case.

Is that accurate?

I guess in this case it's a good thing because it helped expose this issue. But overall, I think this is undesirable.

thearifismail · 2024-06-21T12:37:01Z

Yes tests should be insulated and not rely on which test ran before and after. That said, pytest uses scope for sharing fixtures. Possible values for scope are function, class, module, package, or session.

jpramos123 requested a review from a team as a code owner July 28, 2023 21:11

jpramos123 force-pushed the RHINENG-1284 branch 2 times, most recently from df92987 to 36204de Compare July 31, 2023 13:58

thearifismail reviewed Jul 31, 2023

View reviewed changes

tests/test_api_assignment_rules_create.py Outdated Show resolved Hide resolved

jpramos123 requested a review from thearifismail July 31, 2023 16:46

kruai reviewed Aug 1, 2023

View reviewed changes

jpramos123 requested a review from kruai August 1, 2023 14:20

jpramos123 and others added 4 commits August 1, 2023 11:23

fix(RHINENG-1284): Add constrains to AssignmentRule model

110542c

fix(RHINENG-1284): Fix tests

79d26b2

Apply suggestions from code review

4e90432

Co-authored-by: Muhammad Arif <[email protected]>

Update tests/test_api_assignment_rules_create.py

8cb8470

Co-authored-by: Asa Price <[email protected]>

jpramos123 force-pushed the RHINENG-1284 branch from 4cffaa2 to 8cb8470 Compare August 1, 2023 14:24

FabriciaDinizRH reviewed Aug 2, 2023

View reviewed changes

jpramos123 requested a review from FabriciaDinizRH August 2, 2023 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHINENG-1284] Tests are failing in test_api_assignment_rules_create.py #1440

[RHINENG-1284] Tests are failing in test_api_assignment_rules_create.py #1440

jpramos123 commented Jul 28, 2023

kruai Aug 1, 2023

jpramos123 Aug 1, 2023

kruai Aug 1, 2023

jpramos123 Aug 1, 2023 •

edited

Loading

FabriciaDinizRH Aug 2, 2023

kruai Aug 15, 2023

kruai Aug 1, 2023

jpramos123 Aug 1, 2023

FabriciaDinizRH Aug 2, 2023

FabriciaDinizRH commented Aug 1, 2023

FabriciaDinizRH Aug 2, 2023

jpramos123 Aug 2, 2023

FabriciaDinizRH Aug 8, 2023

FabriciaDinizRH Aug 2, 2023

FabriciaDinizRH Aug 2, 2023

jpramos123 commented Aug 2, 2023

roliveri commented Jun 20, 2024

thearifismail commented Jun 21, 2024

		def _db_get_assignment_rule(ar_id, group_id):
		return AssignmentRule.query.get((ar_id, group_id))

		@@ -38,10 +38,10 @@ def test_get_assignment_rule_by_name(db_create_assignment_rule, db_create_group,


		def test_get_assignment_rule_with_bad_name(db_create_assignment_rule, db_create_group, api_get):

[RHINENG-1284] Tests are failing in test_api_assignment_rules_create.py #1440

Are you sure you want to change the base?

[RHINENG-1284] Tests are failing in test_api_assignment_rules_create.py #1440

Conversation

jpramos123 commented Jul 28, 2023

Overview

PR Checklist

Secure Coding Practices Documentation Reference

Secure Coding Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpramos123 Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabriciaDinizRH commented Aug 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpramos123 commented Aug 2, 2023

roliveri commented Jun 20, 2024

thearifismail commented Jun 21, 2024

jpramos123 Aug 1, 2023 •

edited

Loading