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.

[chris48s]

OK. Thanks for summarising all of that.

• I think as a first step, it seems like Azure Devops and BitBucket are the source of a lot of the acrobatics here and address relatively uncommon cases. I think if we can split those 2 outliers out for now, that gets us down to a core that addresses a lot of fairly common cases which should be easier to think about and review as a first step.

• You've said

  used to bypass issue with compact_message

  Can you explain the problem we're trying to solve here? I can't see anywhere else this is explained.

• With the ignoreOpenApiExample param, it seems like this is here to work round our own self-imposed strictness. To me, it looks like this param just exists to stop us from throwing a TypeError in getBadgeExampleCall(), but if we changed that method to do something like const openApi = serviceClass.openApi || {} then we could drop this option? Does that seem right?

• I don't think we need to worry about content type. That isn't a new option introduced in this PR anyway.

[jNullj]

Sorry for the long time, I'm getting back to this PR.
I'm with you about ignoreOpenApiExample, removed it with 263aba7, We put empty obj at BaseService in openApi so we can test for it. I did keep a warn so we keep track of services missing openApi obj.

Will push a few more commits in the coming days, Will convert to draft while WIP.

[jNullj]

Can you explain the problem we're trying to solve here? I can't see anywhere else this is explained.

The openApi example extraction function did not handle boolean like compact_message correctly as the example value is null while the validation is expecting empty string.
I fixed this with 669d913

[jNullj]

I can actually get rid of content-type by moving it in base services (eg. baseJson, baseXml) into a static veriable.
This would only fail for JetbrainsBase which is a bit odd and uses both xml and json but it appears to be legacy and xml support will be removed as stated here:

/*
JetBrains is a bit awkward. Sometimes we want to call an XML API
and sometimes we want to call a JSON API so we need a mongrel base class.
When the legacy IntelliJ (XML) API is retired we can simplify all this and
switch JetbrainsDownloads, JetbrainsRating and JetbrainsVersion to just
inherit from BaseJsonService directly.
*/

Will add in the next commit

edit: SymfonyInsightBase and Bountysource are unique and override the accepted content-type but it appears the we use the same content-type suffix and it fits the request - is that how it should work?
for example SymfonyInsightBase has content-type of application/vnd.com.sensiolabs.insight+xml but it seems to accept response with content-typeapplication/xml as well

[jNullj]

@chris48s can i split BitbucketRawPullRequests & BitbucketNonRawPullRequests to remove the auth overide option?

[chris48s]

I think I'd still be inclined to just remove the changes related to BitBucket and Azure Devops from this PR for now and think about them separately as follow-ups.

We can still look at them, but I think if we can just look at the relatively common cases first we can probably get a decent subset of this work finished and merged. Then we can think about those two more difficult cases in isolation more clearly.

[chris48s]

Just thinking ahead about BitBucket slightly more: The thing that makes BitBucket awkward to test is self-hosted vs cloud, not full vs raw number so that's what we'd have to split. As it stands, it is hard to make those two services because that is conditional on a query param not a route param.

[jNullj]

I've been working 12-hour days recently, making it hard to complete tasks. With the holidays, I found time to finish this work.

Future PRs will be smaller to avoid bottlenecks.

I've removed Bitbucket as suggested and moved those changes to a separate branch. Although authOverride is currently unused, I've kept it in case we need it for other services until a better solution is found.