Skip to content

add missing unit tests for #2108 #2227

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

Closed
benkroeger wants to merge 2 commits into from
Closed

add missing unit tests for #2108 #2227

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
107da91
add missing unit tests for #2108
benkroeger Apr 13, 2016
c2604c9
add empty line after unit test for better readablility
benkroeger Apr 13, 2016
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
34 changes: 34 additions & 0 deletions test/access-token.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,40 @@ describe('loopback.token(options)', function() {
});
});

it('should overwrite invalid existing token (is !== undefined and has no "id" property) ' +
' when enableDoubkecheck is true',
function(done) {
var token = this.token;

app.use(function(req, res, next) {
req.accessToken = null;
next();
});

app.use(loopback.token({
model: Token,
enableDoublecheck: true,
}));

app.get('/', function(req, res, next) {
res.send(req.accessToken);
});

request(app).get('/')
.set('Authorization', token.id)
.expect(200)
.end(function(err, res) {
if (err) return done(err);
expect(res.body).to.eql({

@bajtos bajtos Apr 13, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it necessary to check all token properties here? I am worried that if we add a new Token property in the future, we will have to fix way too many tests that don't really depend on specific Token properties.

It think it is enough to check that res.body.id is the same as token.id, isn't it?

@benkroeger benkroeger Apr 13, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in my last unit test, I did exactly that - and you commented that only checking on id wouldn't be enough.
Let me know which one you prefer

@bajtos bajtos Jun 13, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see, sorry for me not being consistent! Let's keep what you have now.

id: token.id,
ttl: token.ttl,
userId: token.userId,
created: token.created.toJSON(),
});
done();
});
});

@bajtos bajtos Apr 13, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add an empty line after the test case to serve as a visual delimiter.


it('should overwrite existing token when enableDoublecheck ' +
'and overwriteExistingToken options are truthy',
function(done) {
Expand Down