Skip to content

[TestCases] Added Get Suites#449

Draft
MaciejWiczk wants to merge 19 commits intomasterfrom
feature/add-tc-api
Draft

[TestCases] Added Get Suites#449
MaciejWiczk wants to merge 19 commits intomasterfrom
feature/add-tc-api

Conversation

@MaciejWiczk
Copy link
Collaborator

@pbylicki I'm trying to figure out how to add suites with testcases, but I would need your help on that. I did some WIP code in this PR, but I'm missing logic on hierarchies etc. Could You navigate me?

@MaciejWiczk MaciejWiczk added this to the Test Cases milestone Dec 3, 2021
@MaciejWiczk MaciejWiczk requested a review from pbylicki December 3, 2021 07:38
@pbylicki
Copy link
Owner

pbylicki commented Dec 3, 2021

@pbylicki I'm trying to figure out how to add suites with testcases, but I would need your help on that. I did some WIP code in this PR, but I'm missing logic on hierarchies etc. Could You navigate me?

@MaciejWiczk The main idea there, was that we only add/delete suites as suite hierarchies, not individual suites. That means that endpoint should expect a full suite hierarchy, which is a tree of nested suites, with the tree root being top-level suite, not a part of other suite. It simplifies creation of individual suites and relations between them so that we have easy read access to them and can treat them as regular resource then. Once the suites are saved and you have the IDs returned, adding test cases is straightforward because each test case is linked with specific suite.
So these endpoints should just wrap existing repository methods, with no special logic involved, it's already handled there. There will be more work on CLI side to build the hierarchies from provided suites

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #449 (604474d) into master (1b5b6c8) will decrease coverage by 0.61%.
The diff coverage is 70.58%.

❗ Current head 604474d differs from pull request most recent head e6ebd61. Consider uploading reports for the commit e6ebd61 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   96.24%   95.63%   -0.62%     
==========================================
  Files          46       47       +1     
  Lines        1385     1419      +34     
==========================================
+ Hits         1333     1357      +24     
- Misses         52       62      +10     
Impacted Files Coverage Δ
rfhub2/api/utils/db.py 95.00% <66.66%> (-5.00%) ⬇️
rfhub2/api/endpoints/suites.py 70.00% <70.00%> (ø)
rfhub2/api/router.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b5b6c8...e6ebd61. Read the comment docs.

@MaciejWiczk MaciejWiczk marked this pull request as draft December 8, 2021 13:50
raise HTTPException(status_code=400, detail="Suite does not exist")
try:
db_test_case: DBTestCase = repository.add(DBTestCase.create(test_case))
return repository.get(db_test_case.id)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, and I know I'm missing something. @pbylicki could You take a look?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell what is the error you're facing? I don't see any unit tests for that yet and just by looking at this code it seems to be doing the right thing (map payload to db model -> create record and return it with id -> get full record including suite data by that id)


@router.get("/{id}/", response_model=Suite)
def get_suite(*, repository: SuiteRepository = Depends(get_suite_repository), id: int):
suite: Optional[DBSuite] = repository.get(id)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not DBSuite, repository already maps it to response model

def get_test_case(
*, repository: TestCaseRepository = Depends(get_test_case_repository), id: int
):
testcase: Optional[DBTestCase] = repository.get(id)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not DBTestCase, repository already maps it to response model

suite_repository: SuiteRepository = Depends(get_suite_repository),
test_case: TestCaseCreate,
):
suite: Optional[DBSuite] = suite_repository.get(test_case.suite_id)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not DBSuite, repository already maps it to response model

repository: SuiteRepository = Depends(get_suite_repository),
):
deleted: int = repository.delete_all_hierarchies()
if deleted:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO these delete all endpoints for suites and test cases should return response including number of deleted records, like we do for Keyword Statistics (and also skip the case of returning 404 for 0 rows found).
I know that we have it implemented like this for Collections and I missed it before, but I think my proposal gives more meaningful output that can be also included in CLI logs for visibility.

@@ -0,0 +1,8 @@
from robot.testdoc import TestSuiteFactory
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be deleted



@router.delete("/")
def delete_all_test_cases(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this endpoint needed? When you delete the suites (and I see it is used in CLI), it also deletes related test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants