Improve our approach for testing auth (part 2) - basicAuth

[jNullj]

This PR is follow-up for #9681 and helps to reach the goal of #9493

This PR will include refactor of all basicAuth services:

• bitbucket
• jenkins
• jira
• nexus
• obs - Added in PR Improve our approach for testing auth (part 1) #9681
• sonar
• symfony
• teamcity
• azure-devops

Additional changes to authTest:
4d57607 - Add exampleOverride
f156762 - Add authOverride
cd6c65b - Add configOverrid
cf34fae - Split invoke params into path and query
a713669 - Improve error handling, Throw error when service auth key is missing
99a01e1 - Handle case of userKey without passKey
74554d7 - Add option to ignore openApi example in testAuth
8c38355 - Handle auth.defaultToEmptyStringForUser
57163eb - Add support for multiple requests in service

[jNullj]

This PR is larger then i anticipated, next time i will break down to groups of 1-2 services.

[chris48s]

Yeah - this one looks like a bit of a beast.

Just to set expectations, I'm going to be away on holiday all next week so I may not get to reviewing this for a while - we'll see how it goes. It is on my radar though.

Thanks for your continued help with this project :)

[chris48s]

Hi. Sorry it has taken a while to get back to this.

To start with, I've just been looking at the changes to services/test-helpers.js.
Tbh, I'm a bit worried by the number of slight variants we're introducing into the helper code here.

Here's a question for you:
How many of the services touched in this PR could have been converted to use testAuth() without adding extra optional params in services/test-helpers.js, or is there a subset we could have added with only minimal changes?

I'm not sure I have all the answers, but if every service (or nearly every) service we want to test means we have to add some new optional param to the test helper function to test it, I think that is telling us that we're zeroing in on the wrong abstraction here.

[jNullj]

Hi. Sorry it has taken a while to get back to this.

To start with, I've just been looking at the changes to services/test-helpers.js. Tbh, I'm a bit worried by the number of slight variants we're introducing into the helper code here.

Here's a question for you: How many of the services touched in this PR could have been converted to use testAuth() without adding extra optional params in services/test-helpers.js, or is there a subset we could have added with only minimal changes?

I'm not sure I have all the answers, but if every service (or nearly every) service we want to test means we have to add some new optional param to the test helper function to test it, I think that is telling us that we're zeroing in on the wrong abstraction here.

For scale, in this PR i updated tests of 8 services to use testAuth() with a total of 25 tests.
I mapped each new test and special options used other then service-class, auth method and dummy-response.

Some insight:

1. Config override is the most used option, this is relevant for all services with an option (or even a required) server url. To test these services we must set the authOrigin in config for the test, but while i could try to add the detection for needing to use config-override in those cases automatically, i cant think of a robust way of detecting which service class requires that. For example while most services in this pr uses server query param jira uses jobUrl, so i think that using the server openapi param might result in issues. I could still attempt to make those changes if you think its better to avoid additional options required by the dev, with the higher risk of having issues once in a service.
2. Regarding content-type i think i could remove that option by using getPrototypeOf() or introducing a new type property to the BaseService class and set it values in each class type to hint at the response header required, let me know which of these sounds like the better options, i think using getPrototypeOf is harder to understand for someone new to that code, but comes with the benefit of not effecting production code (less memory as we dont add new property).
3. ignoreOpenApiExample is used due to non-existing openapi example for the tested class, in this case this is expected as its a parent of the services. We could use it every time openapi example is missing but that would lead to confusion imo as the dev writing the test won't easily know why his text misbehaves. I think its better to keep it an intentional option to both signal the dev this example is unique and to force the dev to understand the crafted new test should be unique. I think its better if we think of a good way to document that properly.
4. Multiple outputs - cant think of a way to automate this, i think if we had a way it would be better then an option. It's relevant to any service that uses more then one request.
5. auth override - testAuth is extracting the auth params from the service class auth property, but bitbucket is unique and uses 2 properties. This is a unique class and i doubt we be required to do that often, let me know what do you think we could do with that one?
6. example override - Required every time openapi is ignored, the second and more common use case is for issues with openapi example.
   6.1. AzureDevOpsTests & JenkinsTests - used to bypass issue with compact_message
   6.2. BitbucketRawPullRequests - openapi example is to use hosted server, but we also want to test code branch of cloud host.
   6.3. SonarGeneric - used due to ignoreOpenApiExample (missing open api example)
   6.4. SonarViolations - openapi example uses the less common use case so an override is used to test the common case

Should we focus on another iteration where we try to reduce some options i mentioned above or should we focus more on new ways to tackle the larger issue?

Its hard to tell if we will see many more options used in the tests not yet converted or not before attempting to add those tests.