Begin porting OpenIddictServerBuilderTests from 2.x. #970
Conversation
- Some tests would not work without the changes to how the Options were retrieved. - The skipped tests are those that are currently failing on all frameworks. Will be continuing to try to fix those. - The commented out tests have compiler errors. Will need to go through and see which, if any, are still valid. Related Work Items: openiddict#894
kevinchalet
changed the title
|
@kevinchalet One of the tests I was having trouble with were the certificate ones. What is the process used for generating the certificates used in the test projects? Is it just a self-signed cert created locally? Reviewing your other comments now. |
The certificates stored as embedded resources, they are indeed self-signed certificates. Assuming you're referring to the 2 skipped tests - that use certificate generation - I suspect that's because the |
|
Yeah, I've got that line in there, but currently I don't have a certificate in the project. I was initially thinking I'd need to generate a new one, but I guess it should also be possible to just go back and get the one that was previously in the test project back in 2.x. That might be easier/less prone to new issues popping up in the test. |
Remaining commented out tests are for methods that do not compile as the test is currently written. Need to investigate whether these were truly removed or if maybe the namespace the extensions come from are just not imported yet.
|
|
||
| private class CustomContext : BaseContext | ||
| { | ||
| /// <summary> |
There was a problem hiding this comment.
You can remove the XML comments, they are not very useful in this case.
There was a problem hiding this comment.
Resharper generated those when I used its 'generate missing members' functionality. I can remove them if you like, but it didn't take any time to add them either :).
| // Arrange | ||
| var services = CreateServices(); | ||
| var builder = CreateBuilder(services); | ||
| Action<OpenIddictServerHandlerDescriptor.Builder<CustomContext>> configuration = null; |
There was a problem hiding this comment.
To simply things, feel free to remove that and do builder.AddEventHandler<CustomContext>(configuration: null) like in the other tests.
There was a problem hiding this comment.
I used the variable so that I could use nameof(configuration) in the assertion, rather than hardcoding the string value for the assert. I avoid hardcoding strings when possible - do you think there's a negative to using nameof in this way in the context of a unit test?
There was a problem hiding this comment.
It's certainly not a bad practice, it's just that I haven't used this approach anywhere else, so it's a bit inconsistent. But I'm fine keeping it this way if you like it.
I think I've got most of the coverage of the ServerBuilder class now. Anything related to the certificates or encryption may need to be built out a bit more. I've shied away those due to a lack of experience working with certs/encryption. Regarding the other classes which are referenced in #894 , I intend to start on those next. Do you have any preference between:
|
|
FYI, the fact you're using
This is not blocking for this PR, but to reduce the noise in VS, you'll want probably want to avoid the intermediate variable and do
Great. I'll take a look once this PR is merged.
No preference. Feel free to opt for the one who better suits your flow. If you opt for the second option, please let me know and I'll merge this PR today. Thanks! |
|
Updated to clean up those info messages. I've had info messages turned off in VS for ages so I had no idea it was polluting those up so much. As for the PR, I prefer to merge it in then. Easier to make a new branch then and work on the next set of tests that need ported, and it allows the tests to be in place as the safety net they hopefully will act as. |
|
PR merged, thanks a lot for your contribution! 👏 |
Will be continuing to try to fix those.
Will need to go through and see which, if any, are still valid.
Related Work Items: #894
Posting this in a somewhat WIP state in case @kevinchalet has any feedback on the changes so far, and also to see what happens with the CI builds.