Open paulmelnikow opened 5 years ago
:+1: I think last time we discussed this, we agreed the terminology we should use is:
class NpmVersion
is an integrationI think what you've suggested there broadly fits with that. The only thing that is slightly unfortunate about it is that calling the tests for our integrations "integration tests" is slightly confusing because "integration testing" is a thing. :thinking:
I don't have any insight into the historical discussions/perspectives on this topic, but I personally didn't find the notion of Service classes (that provide the functionality of integrating with a 3rd party service) confusing nor ambiguous. I've found the pattern of using Service classes that encapsulate the functionality of integration pretty common.
The only thing that is slightly unfortunate about it is that calling the tests for our integrations "integration tests" is slightly confusing because "integration testing" is a thing. π€
On the plus side, given our testing strategy, the purpose of the tests that use ServiceTester is substantially the same as integration tests: to exercise our code as it interacts with the live upstream service. So that may run in our favor as we're trying to explain these tests to people (and will help them understand why they are often flaky).
Of course some of those ServiceTester tests use mocks. Though I think that is and should continue to be something of an exception. I think some tighter tests of transform
, render
, and in extreme cases even fetch
could be used to verify those use cases.
Yes that's true - it would be even more confusing if our "integration tests" were actually pure unit tests :laughing:
FTR I'd like to push this to closure one way or another but don't feel strongly about it.
(I think "service," "service family," and "upstream service" are kinda sorta tolerable. I feel more strongly about most of what's in the roadmap in #2846.)
Yeah and I don't mean to be a contrarian π If this has already been discussed and generally agreed upon then I'm good to go π as I'm not opposed to any of the proposed changes. Just wanted to note that I didn't find the nomenclature used for the service classes confusing.
All good on my side, @chris48s's example using NPM makes perfect sense to me. π
Okay, so how do we go about this? I updated the proposals in the top post. Thoughts on those?
This came up again organically in a recent PR discussion (https://github.com/badges/shields/pull/6009#discussion_r550785567) so wanted to follow up here.
There's a couple aspects that I think are important but couldn't recall whether they've come come up in these past discussions:
There's terms like Model
, Repository
, and yes Service
(amongst many others) that are fairly common, particularly with certain design patterns, and thus connote certain meaning that most developers will understand.
So while it's absolutely true that the upstream systems of record can be accurately described as "services" (we also use the terms platforms, endpoints, APIs at times), I don't think that inherently makes the "service" nomenclature within our codebase wrong or confusing, nor should it necessarily prohibit the ability to utilize that term for elements within the source code.
Those elements within our source code that we name and oft refer to as "service classes" do indeed enable the Shields service to provide one or more badges which utilize data from an upstream service/platform, and I can see how that could be holistically be viewed as each respective class providing the ability for Shields to "integrate" with said upstream service (or a subset of that service).
However, as we all know down at the source code level the "integration" term carries certain well known (or at least common) connotations. Each of these classes is doing a lot of different things (defining shields server route(s), setting badge attributes, etc.) including the actual "integration" or interaction with an upstream system of record, and throughout the codebase we also of course have our integration tests.
I think the challenge I'm having with the proposed verbiage changes is that I feel like we're trying to merge these two distinct (IMO) things into one, but that overlap/conflict occurs because the term "integration" means different things in these two areas.
I'd propose that we either consider adding a 4th tier, e.g. something like
class NpmVersion
(and whether we continue to use the term service
at this level for class/directory/file names becomes separate and thus orthogonal)Or that if we'd prefer to stick with a singular term for the holistic/logical function and the code-level names that we search for something different than integration
. For example, I know plugin
is admittedly an overloaded term itself, but it seems like it could be applicable too and IMO is less likely to have the same overlap/conflicts as the integration
term
I'd be on board with 'plugin'.
Integration has the disadvantage that https://github.com/badges/shields/blob/master/services/github/github-api-provider.integration.js is an integration test, whereas https://github.com/badges/shields/blob/master/services/github/github-all-contributors.tester.js is a test for an integration, and they're slightly different.
There's not really anything else in the shields codebase we call a 'plugin'.
Here's a quiz. Is a "service":
The maintainers have discussed offline that this nomenclature is ambiguous.
The word "badge" has a similar problem: is the badge the result, or the code that generates it? I think my favorite of the proposals was @PyvesB' suggestion to adopt the word "integration".
Here are a few related proposals:
.service.js
files to.integration.js
.*.integration.js
to*.core-integration-test.js
?services/
for the code tree? Rename the service-badge issue label to services.services/
tointegrations/
and rename service-badge to integrations.Thoughts?