Feature: signed auth #30
Conversation
Also refactors to make passing opts to stage easier.
| :headers json-headers} | ||
| query-res (api-post :history query-req)] | ||
| (is (= 200 (:status query-res)) | ||
| (str "History query response was: " (pr-str query-res))) |
There was a problem hiding this comment.
I think the purpose of the strings supplied to testing and is is primarily for documentation. It's most important that they explain what should happen, because we can figure out what actually happened pretty easier.
There was a problem hiding this comment.
Good point, I had just copied what was there. I've updated them now.
There was a problem hiding this comment.
I don't think I agree with this. The second string arg to is has been most useful to me when it shows what did happen w/ more info than the test failure alone. It's fine if it also says what should have happened, but that's already pretty obvious from the test itself (at least it should be). You only ever see it when the test fails, so it's a nice way to give some more details about the failure. So I would say the exact opposite: figuring out what should happen is usually obvious (the test failure report says "I expected X but got Y"), but figuring out why what did happen happened isn't always. And this is a good place to provide that info.
There was a problem hiding this comment.
For example:
If this test is changed to document what should have happened, then on failure you'll see (roughly):
Expected: 200
Actual: 400
Response is successful
😕
There was a problem hiding this comment.
It's most important that they explain what should happen
This is also my understanding of the is string convention. But to avoid redundancy with the diff you get anyway, I think they're best used to give some kind of context/explanation for what was expected (and sometimes why) in human terms.
I agree that a string of "Response should be successful" is not useful. But something like "Policy should have allowed XYZ, because ABC" is closer to what I'd expect. Which in this case may not seem the most illustrative either?, but that's the idea as I understand it.
There was a problem hiding this comment.
I don't feel strongly one way or another. I think putting what went wrong in the is 3rd arg makes reading the test source code weirder. I also think it makes the test output easier to read.
However, I'd like to get this merged so Andrew can incorporate these changes in our docs. Would it be acceptable to make a ticket to decide test conventions so we can apply them consistently? I know we have a ton of tests that describe the expected behavior in is, so I don't think we necessarily need to block this PR on that decision.
There was a problem hiding this comment.
I agree with that. I have some thoughts on this stuff that we can discuss in a different forum, but we shouldn't hold back this pr one way or another.
There was a problem hiding this comment.
To be clear, I'm not advocating for putting "what went wrong" in the second is arg. I'm advocating for putting any additional context, etc. that will help determine why the test failed.
And yes, we can move discussion to a future ticket. I think there is currently a mix of approaches here in our source code and unfortunately the example in the official docs is basically useless for determining the intent of the arg.
These are some changes that needed to be made in preparation for https://github.com/fluree/core/issues/57
I noticed that we weren't passing in the verified :did opt for the transact endpoint, so I added that support back in and some tests to verify that it works with both credentials and JWSs.