Icon test refactoring

[hiendv]

Expected Behavior

Effortless tests with icons

Current Behavior

There are 6374 lines of code in icons.test.js

Possible Solution

We could create another fixture containing paths exported from octicons/build/data because the path is the only variable.

[ghost]

It is my fault. I will refactor tests later today. Maybe, we can import path from icon file:

const icon = require('./lib/icons/alert');
const path = icon.data['path'];

or I should create separatly file, that contains all paths? I need your advice, @hiendv :thinking:

[ghost]

what about this :thinking:

[hiendv]

No, please don't take it seriously. It's none of your faults. I'm the one who asked for help and you did a great job writing these tests :smile: Anyway, here is my solution.

External: octicons/build/data

{
  "alert": {
    "keywords": ["warning", "triangle", "exclamation", "point"],
    "path": "<path fill-rule=\"evenodd\" d=\"M8.865 1.52c-.18-.31-.51-.5-.87-.5s-.69.19-.87.5L.275 13.5c-.18.31-.18.69 0 1 .19.31.52.5.87.5h13.7c.36 0 .69-.19.86-.5.17-.31.18-.69.01-1L8.865 1.52zM8.995 13h-2v-2h2v2zm0-3h-2V6h2v4z\"/>",
    "height": "16",
    "width": "16"
  },
  "arrow-down": {
    "keywords": ["point", "direction"],
    "path": "<path fill-rule=\"evenodd\" d=\"M7 7V3H3v4H0l5 6 5-6z\"/>",
    "height": "16",
    "width": "10"
  },
  "arrow-left": {
    "keywords": ["point", "direction"],
    "path": "<path fill-rule=\"evenodd\" d=\"M6 3L0 8l6 5v-3h4V6H6z\"/>",
    "height": "16",
    "width": "10"
  },
  // ...
}

fixtures/icons.js

// we could reexport the original data or
// we could change a little bit like this
const octicons = require('octicons/build/data')
exports.names = Object.keys(octicons).map(octicon => octicon.name) // This is wrong. See 34#issuecomment-335347217
exports.paths = Object.keys(octicons).map(octicon => octicon.path)
// I hate iterating through object properties so ... +1 for this approach

tests

// Old tests
const icons = require('./fixtures/icons.js').names

// New tests which need paths
const paths = require('./fixtures/icons.js').paths

Any opinions? @andpawlenko

[ghost]

@hiendv my built-in JavaScript compiler says that property "name" will be undefined:

exports.names = Object.keys(octicons).map(octicon => octicon.name) // octicons is a plain array of strings here, right?

Regarding on your data.js file pattern, it may be something like this

const octicons = require('octicons/build/data')
exports.names = Object.keys(octicons)
exports.paths = Object.keys(octicons).map(octicon => octicons[octicon]['path'])
// maybe will be better export all icon data: 
// exports.data = Object.keys(octicons).map(octicon => octicons[octicon])

But how can I test different scales and additional class attribute in this case? Maybe we can use another Jest API methods such as stringContaining() or stringMatching() :thinking: I'm got a stuck on this :thinking:

[hiendv]

@andpawlenko Sorry, my bad. It's supposed to be

const octicons = require('octicons/build/data')

exports.names = Object.keys(octicons)
exports.paths = Object.keys(octicons).map(name => octicons[name].path) // This is bad anw

Wait a minute, I'm gonna re-write a little bit.

P/s: The only part which changes is the <svg> markup, the <path> remains so we could easily assert like this

expect(zap.svg()).toBe(`<svg version="1.1" width="10" height="16" viewBox="0 0 10 16" class="octicon octicon-zap" aria-hidden="true" >${paths.zap}</svg>`)

[hiendv]

Notice: Please fire your PR to branch refactor-test @andpawlenko OK. I updated the fixtures. You could write tests like this

import { icons, paths } from './fixtures/icons'
icons.forEach(name => {
  describe(name, () => {
    const icon = require(`../lib/icons/${name}.js`)
    test(`is a valid octicon`, () => {
      expect(icon).toEqual(expect.objectContaining({
        name,
        svg: expect.any(Function)
      }))
    })

    describe('svg', () => {
      test(`returns a svg`, () => {
        expect(icon.svg()).toBe(`<svg version="1.1" width="10" height="16" viewBox="0 0 10 16" class="octicon octicon-${name}" aria-hidden="true" >${paths[name]}</svg>`)
      })
    })
  })
})

[hiendv]

@andpawlenko Sorry, I forgot the dimensions. Well, we should go with the other option: reexport the original data. So now, after this commit it's gonna be

import { icons, default as octicons } from './fixtures/icons'
icons.forEach(name => {
  describe(name, () => {
    const icon = require(`../lib/icons/${name}.js`)
    const octicon = octicons[name]
    test(`is a valid octicon`, () => {
      expect(icon).toEqual(expect.objectContaining({
        name,
        svg: expect.any(Function)
      }))
    })

    describe('svg', () => {
      test(`returns a svg`, () => {
        expect(icon.svg()).toBe(`<svg version="1.1" width="${octicon.width}" height="${octicon.height}" viewBox="0 0 ${octicon.width} ${octicon.height}" class="octicon octicon-${octicon.name}" aria-hidden="true">${octicon.path}</svg>`)
      })
    })
  })
})

[ghost]

@hiendv yep, some octicons have different width and height. That's why I'm worried about different scale tests :new_moon_with_face:

[hiendv]

@andpawlenko I updated my latest comment :smile:. Even normal tests would fail without any information about icon dimensions :rofl:

[ghost]

@hiendv what about this scale test?

test(`returns a svg which scales`, () => {
  const { width, height } = octicon
  const scale = 2

  expect(icon.svg({ scale })).toBe(`<svg version="1.1" width="${width * scale}" height="${height * scale}" viewBox="0 0 ${octicon.width} ${octicon.height}" class="octicon octicon-${octicon.name}" aria-hidden="true">${octicon.path}</svg>`)
})

[ghost]

or with fractional scale:

test(`returns a svg which scales`, () => {
  const { width, height } = octicon
  const scale = 0.5678
  const actualWidth = width * scale
  const actualHeight = height * scale

  expect(icon.svg({ scale })).toBe(`<svg version="1.1" width="${actualWidth.toFixed(2)}" height="${actualHeight.toFixed(2)}" viewBox="0 0 ${octicon.width} ${octicon.height}" class="octicon octicon-${octicon.name}" aria-hidden="true">${octicon.path}</svg>`)
})

[hiendv]

They look good to me. Just a little modification.

const actualWidth = (width * scale).toFixed(2)
const actualHeight = (height * scale).toFixed(2)

[ghost]

Already working on this.