Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REUSE] Add service badges #6330

Merged
merged 5 commits into from Apr 1, 2021
Merged

[REUSE] Add service badges #6330

merged 5 commits into from Apr 1, 2021

Conversation

tapanchudasama7
Copy link
Contributor

This PR fixes issue #4365.

@shields-ci
Copy link

shields-ci commented Mar 28, 2021

Messages
📖 ✨ Thanks for your contribution to Shields, @tapanchudasama!

Generated by 🚫 dangerJS against 413f7a0

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for having a look at this

services/reuse/reuse-compliance.service.js Outdated Show resolved Hide resolved
services/reuse/reuse-compliance.service.js Outdated Show resolved Hide resolved
services/reuse/reuse-compliance.service.js Outdated Show resolved Hide resolved
services/reuse/reuse-compliance.service.js Show resolved Hide resolved
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6330 March 28, 2021 15:15 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6330 March 28, 2021 15:51 Inactive
@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Mar 28, 2021
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6330 March 31, 2021 05:48 Inactive
label: 'reuse',
message: 'compliant',
color: 'green',
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great now. Only one final thing to address and we are done:

In the service tests the reason we run integration tests against live APIs is because we use so many external services, we want tests that fail if the upstream API goes away or makes some change that we need to respond to. We run them all once a day. We will never notice this stuff otherwise. So for this reason we usually want to accept a range of valid values in the live tests. i.e: if github.com/fsfe/reuse-tool becomes non-compliant at some point in futre, it doesn't mean this code is broken - it just means something in this project we are using for testing changed.

Becuase the input values and output values are the same, lets pull out

const isReuseCompliance = Joi.string()
  .valid('compliant', 'non-compliant', 'checking', 'unregistered')
  .required()

into reuse-compliance-helper.js

and then use it in the schema:

const responseSchema = Joi.object({
  status: isReuseCompliance,
}).required()

and to validate the response in the integration test:

t.create('valid repo')
  .get('/github.com/fsfe/reuse-tool.json')
  .expectBadge({
    label: 'reuse',
    message: isReuseCompliance,
  })

and then you could either add another mocked service test for the valid case, or you could just write a small unit test for the helper function in a reuse-compliance-helper.spec.js instead of doing it with mocked HTTP calls in the service tests. I'll leave it with you...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! That is a nice observation! I'll make these changes.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6330 April 1, 2021 06:25 Inactive
@chris48s chris48s merged commit 1eba958 into badges:master Apr 1, 2021
@chris48s
Copy link
Member

chris48s commented Apr 1, 2021

Thanks for your first contribution 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants