Shields: Add a helper for service test expectations

[paulmelnikow]

Our service tests could be made more concise and consistent by adding an expectBadge helper to the Frisby objects we create.

It could look like this:

  .expectBadge({ label: 'something', message: 'something-else', color: 'blue' })

It could translate this into multiple assertions on the response, and handle literals and Joi schemas interchangeably. It could also magically translate color aliases to their actual color, so that we don't have to import colorscheme.

I'd rather not specify ?format=_shields_test everywhere, so maybe we can do this in conjunction with #2686.

[paulmelnikow]

Some unexpected behavior related to our current setup: https://github.com/badges/shields/pull/2780#issuecomment-454879950

[PyvesB]

I'm happy to look into this. However, I suggest we tackle the field renaming and color addition parts in #2686 alone, I'm expecting the diff to be quite big already.

What I have in mind for now is to implement a helper that translates calls similar to the following:

.expectJSON({ name: 'something', value: 'something-else' })
.expectJSONTypes(Joi.object().keys({ name: 'build', value: isBuildStatus }))
.expectJSONTypes(Joi.object().keys({ name: 'build', value: isBuildStatus, color: 'brightgreen' })

to;

.expectBadge({ name: 'something', value: 'something-else' })
.expectBadge({ name: 'build', value: isBuildStatus })
.expectBadge({ name: 'build', value: isBuildStatus, color: 'brightgreen' })

[paulmelnikow]

Thanks for picking this up.

Hmm, I'm not quite sure I'm following how you're proposing to split up the work. Could you clarify what order of operations you're suggesting?

[PyvesB]

I'm suggesting this ordering:

• leave the JSON response schema unchanged but introduce a test helper.
• modify the JSON response schema in #2686 and tweak the test helper to operate on the new field names and an always present color field.

Does this make sense?

[paulmelnikow]

For the most part, yes!

The thing I'm not clear about is whether you want the calls to use the modern key names:

  .expectBadge({ label: 'something', message: 'something-else', color: 'blue' })

or the legacy key names:

  .expectBadge({ name: 'something', value: 'something-else', color: 'blue' })

[PyvesB]

The old ones, as I don't intend on changing the JSON response.

[paulmelnikow]

Wouldn't it be possible to leave the JSON response alone as you intend, invoke the helper using the new keys, and perform a mapping inside the helper?

The assertions could be modified in a few passes, though since each one will need to be touched, it seems like it would make sense to bring the names into alignment at the same time.

[PyvesB]

I can do that as well. The diff will just be much bigger and therefore somewhat harder to review. :smile:

[paulmelnikow]

Sure, makes sense.

Merge conflicts is another consideration. I think as long as it's not all 1000 service tests at once, it should be okay. I'm usually able to respond to code reviews quickly if need be.

[PyvesB]

Okay, I think I'm pretty much done here. Only two tests still use expectJSON in _endpoint.tester.js_. However, they check unusual fields such as labelColor or logoWidth in the JSON response. I don't think it's worth modifying the helper to support these two edge cases, and I suspect they might need to change anyway as part of #2686. I would like us to be able to get rid of the _style=shields_test_ flag and have the JSON systematically return label, message and color, so maybe we can make the tests check parameters in the SVG instead for those two. Let's think about it!

[paulmelnikow]

Thanks so much for picking up this work! The new badges look much cleaner and easier to learn, and should make refactoring easier.

Agreed about getting rid of _shields_test.

Though, I think it would be good if the result of the JSON badge could be used as input to makeBadge to produce the identical result, which I think would require tossing in labelColor, logoWidth, etc.

[PyvesB]

Agreed about getting rid of _shields_test.

Though, I think it would be good if the result of the JSON badge could be used as input to makeBadge to produce the identical result, which I think would require tossing in labelColor, logoWidth, etc.

The input of makeBadge is quite different and not really easy to work with:

{
  format,
  template,
  text,
  colorscheme,
  color,
  colorA,
  colorB,
  labelColor,
  logo,
  logoPosition,
  logoWidth,
  links
}

In particular the label and message fields we use everywhere in tests are "hidden" in the text array.

We could try to align things with the schema for new-style services, which is also what we use for the endpoint badge:

{
  isError,
  label,
  message,
  color,
  labelColor,
  namedLogo,
  logoSvg,
  logoColor,
  logoWidth,
  logoPosition,
  cacheSeconds,
  style
}

However, at the point of filling the JSON template, I think that quite a few of these values are not available as they've already been transformed.

For now, I'm inclined to stick with what you described in #2686 for now, adding label and message as synonyms and adding color.

[paulmelnikow]

However, at the point of filling the JSON template, I think that quite a few of these values are not available as they've already been transformed.

Ah, right, I forgot that we still aren't using label and message in makeBadge.

Indeed, logo resolution is a transformation that happens first, though I'd like to move that responsibility into makeBadge.

For now, I'm inclined to stick with what you described in #2686 for now, adding label and message as synonyms and adding color.

It doesn't hurt to make that change now!