New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests, SQL refactor and fixes for commit_unless_managed #170

Merged
merged 4 commits into from Mar 13, 2017

Conversation

Projects
None yet
3 participants
@atodorov
Contributor

atodorov commented Mar 8, 2017

plus two minor improvements which are also related!

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 8, 2017

Contributor

@tkdchen I prefer if you can merge #168 first because this PR also touches the same files and I want to make sure Travis CI passes before merging this one.

Contributor

atodorov commented Mar 8, 2017

@tkdchen I prefer if you can merge #168 first because this PR also touches the same files and I want to make sure Travis CI passes before merging this one.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 8, 2017

Coverage Status

Changes Unknown when pulling 29f2b49 on MrSenko:fix_148_commit_unless_managed into ** on Nitrate:develop**.

Coverage Status

Changes Unknown when pulling 29f2b49 on MrSenko:fix_148_commit_unless_managed into ** on Nitrate:develop**.

Show outdated Hide outdated tcms/testcases/models.py
TestCasePlan.objects.get_or_create(
case=self,
plan=plan,
)

This comment has been minimized.

@tkdchen

tkdchen Mar 9, 2017

Member

Thought about code style of ORM. For this simple case, would this be more compact

TestCasePlan.objects.get_or_create(case=self, plan=plan)
@tkdchen

tkdchen Mar 9, 2017

Member

Thought about code style of ORM. For this simple case, would this be more compact

TestCasePlan.objects.get_or_create(case=self, plan=plan)

atodorov added some commits Mar 8, 2017

Just use get_or_create() without try-except
the try-except block is doing what get_or_create() is supposed to
do so no need for it!

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Add tests and refactor SQL for XML-RPC TestCase.unlink_plan
also removes the use of commit_unless_managed(). Related to #148

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
Add tests and SQL->ORM refactor for TestPlan.delete_case()
Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 9, 2017

Coverage Status

Changes Unknown when pulling 7ec50d2 on MrSenko:fix_148_commit_unless_managed into ** on Nitrate:develop**.

Coverage Status

Changes Unknown when pulling 7ec50d2 on MrSenko:fix_148_commit_unless_managed into ** on Nitrate:develop**.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Mar 9, 2017

Contributor

@tkdchen I think it is safe to merge this one as well.

Contributor

atodorov commented Mar 9, 2017

@tkdchen I think it is safe to merge this one as well.

# Link the plans to cases
def _generate_link_plan_value():
for plan_id in plan_ids:
for case_id in case_ids:
yield plan_id, case_id
if (plan_id, case_id) not in existing:

This comment has been minimized.

@tkdchen

tkdchen Mar 12, 2017

Member

There is a unique constraint in table to avoid inserting duplicate pair of plan and case. This check could be a good way, but it changes original behavior of this API. Currently, I would not prefer to change original behavior of any API even if it's really good to do that. All of them the good things should go to new version of API whatever XMLRPC API or new RESTful API.

BTW, are you using XMLRPC API now?

@tkdchen

tkdchen Mar 12, 2017

Member

There is a unique constraint in table to avoid inserting duplicate pair of plan and case. This check could be a good way, but it changes original behavior of this API. Currently, I would not prefer to change original behavior of any API even if it's really good to do that. All of them the good things should go to new version of API whatever XMLRPC API or new RESTful API.

BTW, are you using XMLRPC API now?

This comment has been minimized.

@atodorov

atodorov Mar 12, 2017

Contributor

There is a unique constraint in table to avoid inserting duplicate pair of plan and case

Yes, this is what "INSERT IGNORE" was used for, right ?

Because the ORM doesn't have such equivalent I am replacing the "INSERT IGNORE" with a new ORM query which selects the existing IDs from the DB and this check which prevents the bulk_create() method to attempt and insert duplicate records. There is no change in existing behavior, only in existing implementation. That's why there is the test_insert_ignores_existing_mappings test case. Does this answer your question ?

And yes I am using the XML-RPC extensively.

@atodorov

atodorov Mar 12, 2017

Contributor

There is a unique constraint in table to avoid inserting duplicate pair of plan and case

Yes, this is what "INSERT IGNORE" was used for, right ?

Because the ORM doesn't have such equivalent I am replacing the "INSERT IGNORE" with a new ORM query which selects the existing IDs from the DB and this check which prevents the bulk_create() method to attempt and insert duplicate records. There is no change in existing behavior, only in existing implementation. That's why there is the test_insert_ignores_existing_mappings test case. Does this answer your question ?

And yes I am using the XML-RPC extensively.

This comment has been minimized.

@tkdchen

tkdchen Mar 13, 2017

Member

Yes, this is what "INSERT IGNORE" was used for, right ?

Yes. I didn't notice it. So, the change is okay.

And yes I am using the XML-RPC extensively.

Good to know this.

@tkdchen

tkdchen Mar 13, 2017

Member

Yes, this is what "INSERT IGNORE" was used for, right ?

Yes. I didn't notice it. So, the change is okay.

And yes I am using the XML-RPC extensively.

Good to know this.

Show outdated Hide outdated tcms/xmlrpc/api/testcase.py
objects_to_insert = []
for _plan_id, _case_id in _generate_link_plan_value():
objects_to_insert.append(TestCasePlan(plan_id=_plan_id, case_id=_case_id))
TestCasePlan.objects.bulk_create(objects_to_insert)

This comment has been minimized.

@tkdchen

tkdchen Mar 12, 2017

Member

Same idea as my previous comment, to make these lines more compact and readable

TestCasePlan.objects.bulk_create([
    TestCasePlan(plan_id=_plan_id, case_id=_case_id)
    for _plan_id, _case_id in _generate_link_plan_value()
])

or, it is also okay to split it into two lines

case_plan_rels = [
    TestCasePlan(plan_id=_plan_id, case_id=_case_id)
    for _plan_id, _case_id in _generate_link_plan_value()
]
TestCasePlan.objects.bulk_create(case_plan_rels)
@tkdchen

tkdchen Mar 12, 2017

Member

Same idea as my previous comment, to make these lines more compact and readable

TestCasePlan.objects.bulk_create([
    TestCasePlan(plan_id=_plan_id, case_id=_case_id)
    for _plan_id, _case_id in _generate_link_plan_value()
])

or, it is also okay to split it into two lines

case_plan_rels = [
    TestCasePlan(plan_id=_plan_id, case_id=_case_id)
    for _plan_id, _case_id in _generate_link_plan_value()
]
TestCasePlan.objects.bulk_create(case_plan_rels)

This comment has been minimized.

@atodorov

atodorov Mar 12, 2017

Contributor

IMO nesting statements inside each other, especially loops isn't more readable neither more compact but suit yourself.

@atodorov

atodorov Mar 12, 2017

Contributor

IMO nesting statements inside each other, especially loops isn't more readable neither more compact but suit yourself.

Add test and refactor SQL for XML-RPC testcase.link_plan()
this commit uses bulk inserts for performance reasons but does
add one extra query to make sure we're not inserting duplicate
records into the 'test_case_plans' table!

This makes the new code more portable between database engines and
also removes the use of commit_unless_managed().

Fix #148

Signed-off-by: Mr. Senko <atodorov@mrsenko.com>
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 12, 2017

Coverage Status

Coverage increased (+0.2%) to 45.402% when pulling ad0bef3 on MrSenko:fix_148_commit_unless_managed into ae3745a on Nitrate:develop.

Coverage Status

Coverage increased (+0.2%) to 45.402% when pulling ad0bef3 on MrSenko:fix_148_commit_unless_managed into ae3745a on Nitrate:develop.

@tkdchen tkdchen merged commit 980b07b into Nitrate:develop Mar 13, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment