Improve code coverage #125
Conversation
| $this->connector->shouldReceive('getAll') | ||
| ->once()->andReturn([]); | ||
|
|
||
| $result = $service->findLineItem($lineItem); |
There was a problem hiding this comment.
This is a totally unrelated question but why dont we compare lineitem id in LtiAssignmentsGradeService::isMatchingLineitem?
There was a problem hiding this comment.
Great question. For background, here's the discussion about it on the PR that introduced it: #35 (comment)
Ok, it took me a while to figure out, but I think I remember now finally. It's because when we're syncing grades, our app doesn't necessarily know if the line item exists in the LMS yet. As such the ID could be empty.
Both the getGrades() and putGrade() methods call ensureLineItemExists() which calls findOrCreateLineItem() which calls findLineItem() which calls isMatchingLineItem().
In the case where we call get|putGrade[s]() and we need to create the line item still, we won't have an ID. (We will, however, have the tag, resource ID, and resource link ID.) Hence why we don't match on ID.
Tho I suppose we could add a $newLineItem->getId() == ($lineitem['id'] ?? null) || to the beginning of isMatchingLineitem()
|
|
||
| $expected = new LtiLineitem($ltiLineitemData); | ||
|
|
||
| $result = $service->createLineItem(new LtiLineItem()); |
There was a problem hiding this comment.
For consistancy, shouldn't this and other methods that make a service request validate scopes before making the request?
There was a problem hiding this comment.
This is a good suggestion, but probably requires a more holistic refactor. Are you ok with me addressing that in a separate PR?
| @@ -76,6 +119,131 @@ public function testItGetsSingleLineItemWithReadonlyScope() | |||
| $this->assertEquals($expected, $result); | |||
| } | |||
|
|
|||
| public function testItPutsAGrade() | |||
There was a problem hiding this comment.
Could you test that tis throws an exception if the wrong scope is set?
|
|
||
| $this->assertEquals($expected, $this->key->getSecret()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we add a tests for Lti1p1Key::sign()?
Summary of Changes
Gets us to like 99% code coverage. Also, testing found a legit bug FTW!
Testing
composer testbefore opening this PRcomposer lint-fixbefore opening this PR