Skip to content
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.

Sign up for GitHub

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

Improve code coverage #125

Merged
merged 10 commits into from
Dec 21, 2023
Merged

Improve code coverage #125

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bda6925
Up code coverage
dbhynds Dec 19, 2023
d6463ee
test group sets
dbhynds Dec 20, 2023
5d97afd
Add a bunch of tests
dbhynds Dec 20, 2023
fc16bf2
finish testing grade
dbhynds Dec 20, 2023
1d45eab
Cover a few more lines
dbhynds Dec 20, 2023
5db5b56
Merge branch 'v6.0-beta' into v6-code-coverage
dbhynds Dec 21, 2023
011af76
add a few more tests
dbhynds Dec 21, 2023
c94b94b
add a few more tests
dbhynds Dec 21, 2023
65e528b
add a few more tests
dbhynds Dec 21, 2023
53ef41d
add another tests
dbhynds Dec 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Interfaces/ILtiServiceConnector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public function getAll(
ILtiRegistration $registration,
array $scopes,
IServiceRequest $request,
string $key
?string $key
): array;

public function setDebuggingMode(bool $enable): ILtiServiceConnector;
Expand Down
50 changes: 50 additions & 0 deletions tests/Lti1p1KeyTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<?php

namespace Tests;

use Packback\Lti1p3\Lti1p1Key;

class Lti1p1KeyTest extends TestCase
{
public function setUp(): void
{
$this->key = new Lti1p1Key();
}

public function testItInstantiates()
{
$this->assertInstanceOf(Lti1p1Key::class, $this->key);
}

public function testItGetsKey()
{
$result = $this->key->getKey();

$this->assertNull($result);
}

public function testItSetsKey()
{
$expected = 'expected';

$this->key->setKey($expected);

$this->assertEquals($expected, $this->key->getKey());
}

public function testItGetsSecret()
{
$result = $this->key->getSecret();

$this->assertNull($result);
}

public function testItSetsSecret()
{
$expected = 'expected';

$this->key->setSecret($expected);

$this->assertEquals($expected, $this->key->getSecret());
}
}
Copy link

Choose a reason for hiding this comment

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

Should we add a tests for Lti1p1Key::sign()?

303 changes: 303 additions & 0 deletions tests/LtiAssignmentsGradesServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
use Packback\Lti1p3\Interfaces\ILtiServiceConnector;
use Packback\Lti1p3\LtiAssignmentsGradesService;
use Packback\Lti1p3\LtiConstants;
use Packback\Lti1p3\LtiException;
use Packback\Lti1p3\LtiGrade;
use Packback\Lti1p3\LtiLineitem;

class LtiAssignmentsGradesServiceTest extends TestCase
Expand All @@ -24,6 +26,47 @@ public function testItInstantiates()
$this->assertInstanceOf(LtiAssignmentsGradesService::class, $service);
}

public function testItGetsScope()
{
$serviceData = [
'scope' => ['asdf'],
];

$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$result = $service->getScope();

$this->assertEquals($serviceData['scope'], $result);
}

public function testItGetsResourceLaunchLineItem()
{
$ltiLineitemData = [
'id' => 'testId',
];

$serviceData = [
'lineitem' => $ltiLineitemData['id'],
];

$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$expected = new LtiLineitem($ltiLineitemData);

$result = $service->getResourceLaunchLineItem();

$this->assertEquals($expected, $result);
}

public function testItGetsNullResourceLaunchLineItem()
{
$service = new LtiAssignmentsGradesService($this->connector, $this->registration, []);

$result = $service->getResourceLaunchLineItem();

$this->assertNull($result);
}

public function testItGetsSingleLineItem()
{
$ltiLineitemData = [
Expand Down Expand Up @@ -76,6 +119,131 @@ public function testItGetsSingleLineItemWithReadonlyScope()
$this->assertEquals($expected, $result);
}

public function testItPutsAGrade()
Copy link

Choose a reason for hiding this comment

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

Could you test that tis throws an exception if the wrong scope is set?

{
$serviceData = [
'scope' => [LtiConstants::AGS_SCOPE_SCORE],
'lineitems' => 'https://canvas.localhost/api/lti/courses/8/line_items',
];
$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$lineItem = new LtiLineitem([
'id' => 'testId',
]);

$expected = [
'scoreGiven' => 10,
'scoreMaximum' => 15,
];
$grade = new LtiGrade($expected);
$this->connector->shouldReceive('makeServiceRequest')
->once()->andReturn($expected);

$result = $service->putGrade($grade, $lineItem);

$this->assertEquals($expected, $result);
}

public function testItFindsALineItem()
{
$ltiLineitemData = [
'id' => 'testId',
];

$serviceData = [
'scope' => [LtiConstants::AGS_SCOPE_LINEITEM],
'lineitems' => 'https://canvas.localhost/api/lti/courses/8/line_items',
];

$lineItem = new LtiLineitem($ltiLineitemData);
$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$response = [$ltiLineitemData];

$this->connector->shouldReceive('getAll')
->once()->andReturn($response);

$result = $service->findLineItem($lineItem);

$this->assertEquals($lineItem, $result);
}

public function testItFindsNoLineItem()
{
$ltiLineitemData = [
'id' => 'testId',
];

$serviceData = [
'scope' => [LtiConstants::AGS_SCOPE_LINEITEM],
'lineitems' => 'https://canvas.localhost/api/lti/courses/8/line_items',
];

$lineItem = new LtiLineitem($ltiLineitemData);
$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$this->connector->shouldReceive('getAll')
->once()->andReturn([]);

$result = $service->findLineItem($lineItem);
Copy link

Choose a reason for hiding this comment

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

This is a totally unrelated question but why dont we compare lineitem id in LtiAssignmentsGradeService::isMatchingLineitem?

Copy link
Member Author

Choose a reason for hiding this comment

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

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()


$this->assertNull($result);
}

public function testItUpdatesALineItem()
{
$ltiLineitemData = [
'id' => 'testId',
];

$serviceData = [
'scope' => [LtiConstants::AGS_SCOPE_LINEITEM],
'lineitem' => 'https://canvas.localhost/api/lti/courses/8/line_items/27',
];

$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$response = [
'body' => $ltiLineitemData,
];

$this->connector->shouldReceive('makeServiceRequest')
->once()->andReturn($response);

$expected = new LtiLineitem($ltiLineitemData);

$result = $service->updateLineItem(new LtiLineItem());

$this->assertEquals($expected, $result);
}

public function testItCreatesALineItem()
{
$ltiLineitemData = [
'id' => 'testId',
];

$serviceData = [
'scope' => [LtiConstants::AGS_SCOPE_LINEITEM],
'lineitems' => 'https://canvas.localhost/api/lti/courses/8/line_items',
];

$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$response = [
'body' => $ltiLineitemData,
];

$this->connector->shouldReceive('makeServiceRequest')
->once()->andReturn($response);

$expected = new LtiLineitem($ltiLineitemData);

$result = $service->createLineItem(new LtiLineItem());
Copy link

Choose a reason for hiding this comment

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

For consistancy, shouldn't this and other methods that make a service request validate scopes before making the request?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good suggestion, but probably requires a more holistic refactor. Are you ok with me addressing that in a separate PR?


$this->assertEquals($expected, $result);
}

public function testItDeletesALineItem()
{
$serviceData = [
Expand All @@ -97,4 +265,139 @@ public function testItDeletesALineItem()

$this->assertEquals($response, $result);
}

public function testItFindsOrCreatesALineItem()
{
$ltiLineitemData = [
'id' => 'testId',
];

$serviceData = [
'scope' => [LtiConstants::AGS_SCOPE_LINEITEM],
'lineitems' => 'https://canvas.localhost/api/lti/courses/8/line_items',
];

$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$response = [
'body' => $ltiLineitemData,
];

// Find Line Item
$this->connector->shouldReceive('getAll')
->once()->andReturn([]);
// Create Line Item
$this->connector->shouldReceive('makeServiceRequest')
->once()->andReturn($response);

$expected = new LtiLineitem($ltiLineitemData);

$result = $service->findOrCreateLineitem($expected);

$this->assertEquals($expected, $result);
}

public function testItThrowsWithMissingScope()
{
$ltiLineitemData = [
'id' => 'testId',
];

$serviceData = [
'scope' => [],
];

$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$expected = new LtiLineitem($ltiLineitemData);

$this->expectException(LtiException::class);

$service->getLineItem('someUrl');
}

public function testItSetsServiceData()
{
$expected = ['foo' => 'bar'];

$service = new LtiAssignmentsGradesService($this->connector, $this->registration, []);
$service->setServiceData($expected);

$actual = $service->getServiceData();

$this->assertEquals($expected, $actual);
}

public function testItGetsGrades()
{
$serviceData = [
'scope' => [LtiConstants::AGS_SCOPE_LINEITEM],
'lineitems' => 'https://canvas.localhost/api/lti/courses/8/line_items',
];
$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$lineItem = new LtiLineitem([
'id' => 'testId',
]);

$expected = [[
'scoreGiven' => 10,
'scoreMaximum' => 15,
]];
$this->connector->shouldReceive('getAll')
->once()->andReturn($expected);

$result = $service->getGrades($lineItem);

$this->assertEquals($expected, $result);
}

public function testItGetsLineItems()
{
$serviceData = [
'scope' => [LtiConstants::AGS_SCOPE_LINEITEM],
'lineitems' => 'https://canvas.localhost/api/lti/courses/8/line_items',
];

$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$response = [
'status' => 204,
'body' => [],
];

$this->connector->shouldReceive('getAll')
->once()->andReturn($response);

$result = $service->getLineItems();

$this->assertEquals($response, $result);
}

public function testItGetsALineItems()
{
$serviceData = [
'scope' => [LtiConstants::AGS_SCOPE_LINEITEM],
'lineitems' => 'https://canvas.localhost/api/lti/courses/8/line_items',
];

$service = new LtiAssignmentsGradesService($this->connector, $this->registration, $serviceData);

$response = [
'status' => 204,
'body' => ['id' => 'testId'],
];

$expected = [
'status' => 204,
'body' => [['id' => 'testId']],
];

$this->connector->shouldReceive('getAll')
->once()->andReturn($response);

$result = $service->getLineItems();

$this->assertEquals($expected, $result);
}
}
Loading