Code review: Rspec test refactoring #85
Conversation
| @@ -163,17 +161,17 @@ def decode(jwt, key=nil, verify=true, options={}, &keyfinder) | |||
| if options[:verify_not_before] && payload.include?('nbf') | |||
| raise JWT::ImmatureSignature.new('Signature nbf has not been reached') unless payload['nbf'].to_i < (Time.now.to_i + options[:leeway]) | |||
| end | |||
| if options[:verify_iss] && payload.include?('iss') | |||
| raise JWT::InvalidIssuerError.new("Invalid issuer. Expected #{options['iss']}, received #{payload['iss']}") unless payload['iss'].to_s == options['iss'].to_s | |||
| if options[:verify_iss] && options['iss'] | |||
There was a problem hiding this comment.
this looks like it could lead to problems with implementers:
if the caller passes :verify_iss but not 'iss' (or the other way around), then this could leas to an unexpected successful verification.
Probably worth adding a check and failing with an argument error in the case that one is specified but not both.
There was a problem hiding this comment.
Thanks for the review :)
Going to add more tests and implement checks for these cases.
| expect(decoded_payload).to include(example_payload) | ||
| expect(decoded_payload2).to include(example_payload2) | ||
| end | ||
| it 'should decode a valid token' do |
There was a problem hiding this comment.
it should decode a valid token and it should generate a valid token are identical tests.
There was a problem hiding this comment.
@aj-michael How would you merge these two tests into a single test?
There was a problem hiding this comment.
My apologies, I'm not saying they should be identical tests. I'm saying that at the moment, they are identical, as in they contain the same expressions. I don't believe they should be.
|
Nitpick: can you choose a consistent syntax for hash literals with symbols? grep is counting 23 occurrences of |
|
@aj-michael This happens when I code on the train and look out of the window. Will fix that in upcoming commits. |
Add fixtures/certs path Update configuration
Used wrong curve -.- :/
Test coverage at 91% Few validation specs missing
Remove 'NONE' fix Add more tests for verifications Copy paste new verification code from master
Update code Fix broken tests
Missing OpenSSL functions
excpt
force-pushed
the
rspec-test-refactoring
branch
from
193f67f
to
5ce6d7e
Compare
(cherry picked from commit ee7c24c)
Remove whitespaces
Add two iss claim tests from master branch. Fix a typo.
Code review: Rspec test refactoring
Code review request. Please comment! :)