807 | OpenIddict.Validation.AspNetCore : InferIssuerFromHost tests

[Darthruneis]

Taking a stab at writing some tests for #807 .

I wanted to get an initial test class written and see if there's any feedback before continuing on to more of the handlers inside of OpenIddict.Validation.AspNetCore.

[kevinchalet]

Thanks for your PR, @Darthruneis! 👏

Here's my feeling: when I opened #807 last year (and its server counterpart), I didn't anticipate how big - and time-consuming - 3.0 would become.

I ported the server integration tests and it already took longer than I expected (and while it's essential, writing/porting tests is rarely a fun task 😅). Given how time-consuming this can be, I'm wondering if adding unit tests for the handlers is the right thing to do, as most of the logic they would test would be also covered by the integration tests. Of course, contributions like yours definitely help, but what's unclear is if contributions will cover all the handlers (I'm not so sure), or if I'll have to dedicate time to that to fill in the blanks eventually.

I'd love to hear your thoughts.

[Darthruneis]

@kevinchalet I can't say I'm any good at predicting the future, and thus I can't say how much test coverage will be supplied by contributions. That said, I think having unit tests in addition to integration tests is a nice to have, and will generally cover the units more thoroughly than the integration tests will. Integration and unit tests have different goals, after all.

The tests in this PR, for example. I wouldn't expect to see integration tests around some of the edge cases, such as the null checks. That seems like it would be more time intensive to set up in an integration test than it would in a unit test. Additionally, I can see it becoming a pain point for maintenance, as integration tests tend to be more brittle than unit tests.

Finally, I'm hoping that by starting with unit testing various pieces of code, I'll get up to speed with the repo as a whole, and eventually might be able to contribute in other ways.

I'm quite excited by the ASOS merge that 3.0 is aiming to do, I'd like to help in some way if I can, even if its just unit testing for now :).

[kevinchalet]

I'm quite excited by the ASOS merge that 3.0 is aiming to do, I'd like to help in some way if I can, even if its just unit testing for now :).

Thanks a lot for your interest! 😃

I agree with everything you said. Sadly, I have limited bandwidth these days, and while having unit tests for all the handlers would be valuable, I think focusing on the integration tests first will be a better option: they are indeed less fine-grained and won't really cover corner cases, but they help guarantee that the handlers chain forms a coherent pipeline that reacts exactly how it should, which is not something we can test with per-handler unit tests.

I'll remove #807 and #761 from the next milestone so we can focus on the other tasks first (including porting the validation/introspection TestServer-based tests).

If you're still interested in contributing to the unit tests, maybe you could take a look at #894?

Thanks again!

[kevinchalet]

Closing for now. We'll reconsider later if we have contributions for this area 😃