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

add integration test in CI #151

Closed
wants to merge 11 commits into from
Closed

add integration test in CI #151

wants to merge 11 commits into from

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented Aug 5, 2020

Part of #37

@dtassone
Copy link
Member Author

dtassone commented Aug 6, 2020

Main issue with this one is the font rendering which makes the tests fail due to too many differences between my local image files and the server generated ones.
So I tried overriding the font to monospace for all elements but it didn't work.

I believe the best way to achieve this is to setup our local to use docker and run the test inside the same container image as the server.

Good place to start
https://github.com/vsupalov/docker-puppeteer-dev
https://vsupalov.com/headless-chrome-puppeteer-docker/

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

If we can kill the __image_snapshots__ folder, that will be awesome!
I have added the ARGOS_TOKEN env variable in the Circle CI configuration.

@@ -11,7 +11,7 @@ defaults: &defaults
REACT_DIST_TAG: << parameters.react-dist-tag >>
working_directory: /tmp/material-ui-x
docker:
- image: circleci/node:10
- image: circleci/node:latest-browsers
Copy link
Member

Choose a reason for hiding this comment

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

We prefer to run the tests with v10, as the closest version to the minimum version of node we support for the components. Right now, we support node v8, but plan to upgrade mui/material-ui#19301 (comment).

Also, in theory, we don't need to load this distribution, puppeteer comes with a binary of chrome in node_modules/puppeteer/.local-chromium

Suggested change
- image: circleci/node:latest-browsers
- image: circleci/node:10

For the missing dependencies, we have prepare_chrome_headless the step which should work with the non headless version too

@@ -81,6 +81,7 @@
"test:karma": "cross-env NODE_ENV=test karma start test/karma.conf.js",
"test:unit": "cross-env NODE_ENV=test mocha 'packages/**/*.test.tsx' --exclude '**/node_modules/**'",
"test:watch": "yarn test:unit --watch",
"test:e2e": "cd packages/storybook && yarn test:ci",
Copy link
Member

Choose a reason for hiding this comment

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

Considering storybook is a workspace?

Suggested change
"test:e2e": "cd packages/storybook && yarn test:ci",
"test:e2e": "yarn workspace storybook test:ci",

@@ -31,4 +31,7 @@ describe('Components override', () => {
await snapshotTest('/story/x-grid-demos-custom-components--styled-columns');
done();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
done();

@@ -31,4 +31,7 @@ describe('Components override', () => {
await snapshotTest('/story/x-grid-demos-custom-components--styled-columns');
done();
});
afterAll(async () => {
await stopBrowser();
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused, why are startBrowser and stopBrowser needed? Isn't this feature provided by https://github.com/smooth-code/jest-puppeteer/blob/69a0c3ee80ef4b6af08d45ff44229df6ced8b666/packages/jest-puppeteer-preset/jest-preset.json#L2 in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

No as it's puppeter the web driver so you need to load an instance of the browser from its package.

Copy link
Member

Choose a reason for hiding this comment

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

I don't get it. From what I understand, we use jest-puppeteer, which is meant to handle the launch and close of the browser:

@@ -20,15 +24,33 @@ export const IMAGE_SNAPSHOT_CONFIG = {
},
} as MatchImageSnapshotOptions;

let browserInstance;
export async function startBrowser() {
Copy link
Member

Choose a reason for hiding this comment

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

I had a look at the helpers I wrote in the past to write tests with puppeteer, if that help:

Usage, createContext()

import config from 'config'
import { ACCOUNT_STATUSES, SCOPES } from 'api/constants'
import { createContext } from 'test/e2e/setup'
import signIn from 'test/e2e/modules/signIn'
import matchers from 'test/e2e/modules/matchers'
import screenshot from 'test/e2e/modules/screenshot'

describe('signIn', () => {
  const context = createContext()

  it('should work', async () => {
    const account = await context.factory.create(
      'Account',
      { status: ACCOUNT_STATUSES.APPROVED },
      { scopes: [SCOPES.CONTRIBUTOR] }
    )
    const { page, i18n } = context

    await page.goto(`${config.get('contributor.url')}/sign-in`)
    await matchers.toMatch(page, i18n.t('contributorSignIn:apply'), {
      timeout: 'navigation',
    })
    await screenshot(page, 'contributor-sign-in')
    await page.type('input[name="email"]', account.email)
    await page.type('input[name="password"]', account.password)
    await page.type('input[name="password"]', '\u000d')
    await matchers.toMatchUrl(page, `${config.get('contributor.url')}/`)
    await matchers.toMatch(page, i18n.t('contributorDashboard:imageRequiresEditing'), {
      timeout: 'navigation',
    })
    await page.waitFor('[data-e2e="appBar.user.menu"]')
    await screenshot(page, 'contributor-dashboard')
    await page.click('[data-e2e="appBar.user.menu"]')
    await matchers.toMatchElement(page, '[role="menu"] > div:first-child', {
      text: `${account.firstName} ${account.lastName}`,
    })
  })

  it('should redirect when trying to access the sign in page', async () => {
    const account = await context.factory.create(
      'Account',
      { status: ACCOUNT_STATUSES.APPROVED },
      { scopes: [SCOPES.CONTRIBUTOR] }
    )
    await signIn({ context, account, scope: SCOPES.CONTRIBUTOR })

    const { page } = context
    await page.goto(`${config.get('contributor.url')}/settings/profile`)
    expect(page.url()).toBe(`${config.get('contributor.url')}/settings/profile`)
    await page.goto(`${config.get('contributor.url')}/sign-in`)
    expect(page.url()).toBe(`${config.get('contributor.url')}/`)
  })
})

test/e2e/setup.js

/* eslint-disable no-underscore-dangle, import/prefer-default-export, import/no-extraneous-dependencies */
/* global jasmine */

import fetch from 'isomorphic-fetch'
import knexfile from 'knexfile'
import puppeteer from 'puppeteer'
import log from 'modules/scripts/log'
import waitUntil from 'modules/async/waitUntil'
import hackTranslate from 'modules/i18n/hackTranslate'
import i18nInit from 'web/modules/i18n/server'
import getFactory from 'test/modules/api/factory'
import createKnex from 'api/services/database/createKnex'
import truncateAll from 'api/services/database/truncateAll'
import { IMAGE_STATUSES } from 'api/constants'

jasmine.DEFAULT_TIMEOUT_INTERVAL = 6000e3

async function getBrowserWSEndpoint() {
  if (process.env.__BROWSER_WS_ENDPOINT__) {
    return process.env.__BROWSER_WS_ENDPOINT__
  }

  const browserRemoteUrl = process.env.BROWSER_REMOTE_URL

  if (!browserRemoteUrl) {
    throw new Error('process.env.BROWSER_REMOTE_URL is missing')
  }

  const browserWSEndpoint = await waitUntil(async () => {
    log.info({
      name: 'browser',
      msg: `Fetch data from ${browserRemoteUrl}`,
      force: true,
    })
    let version

    try {
      version = await fetch(`${browserRemoteUrl}/json/version`, {
        method: 'GET',
      }).then(result => result.json())
    } catch (err) {
      // ...
    }

    return {
      predicate: version,
      result: version ? version.webSocketDebuggerUrl : null,
    }
  })

  log.info({
    name: 'browser',
    msg: `Resolved browser endpoint ${browserWSEndpoint} `,
    force: true,
  })

  process.env.__BROWSER_WS_ENDPOINT__ = browserWSEndpoint

  return browserWSEndpoint
}

async function launchBrowser() {
  const browserWSEndpoint = await getBrowserWSEndpoint()
  return puppeteer.connect({ browserWSEndpoint, slowMo: 0 })
}

function handleError(err) {
  throw err
}

const knexConfig = knexfile[process.env.NODE_ENV]

export function createContext() {
  const context = {}

  const screenSize = process.env.SCREEN_SIZE
  if (!screenSize) {
    throw new Error('Missing process.env.SCREEN_SIZE')
  }
  const [width, height] = screenSize.split('x').map(Number)

  beforeAll(async () => {
    // console.time('db')
    context.knex = createKnex({ connection: { database: knexConfig.connection.database } })
    context.factory = getFactory(context.knex.models)
    // console.timeEnd('db')

    await Promise.all([
      truncateAll(context.knex),

      (async () => {
        // console.time('browser')
        context.browser = await launchBrowser()
        const pages = await context.browser.pages()
        if (!pages.length) {
          pages.push(await context.browser.newPage())
        }
        context.page = pages[0]
        context.page.setDefaultNavigationTimeout(
          process.env.NODE_ENV === 'production' ? 10e3 : 60e3
        )
        context.page.addListener('pageerror', handleError)
        await context.page.setViewport({ width, height: height - 83 })
        // console.timeEnd('browser')
      })(),
      (async () => {
        // console.time('i18n')
        const i18n = await i18nInit()
        context.i18n = hackTranslate(i18n)
        // console.timeEnd('i18n')
      })(),
    ])
  })

  // Avoid duplicating beforeAll truncate
  let truncateDo = false
  beforeEach(async () => {
    // Kill all the in-flight requests
    const html =
      '<!DOCTYPE html><html><head></head><body>beforeEach, kill all the pending requests</body></html>'
    await context.page.goto(`data:text/html,${html}`)
    await Promise.all([
      (async () => {
        if (truncateDo) {
          await truncateAll(context.knex)
        }
        truncateDo = true
      })(),
      context.page._client.send('Network.clearBrowserCookies'),
    ])

    await context.factory.create('Image', {
      customerHomeBackground: true,
      status: IMAGE_STATUSES.APPROVED_ONLINE,
    })
  })

  afterAll(async () => {
    context.page.removeListener('pageerror', handleError)
    await Promise.all([context.browser.disconnect(), context.knex.destroy()])
  })

  return context
}

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 8, 2020

Regarding 22d8b0c, here is my experience with doing visual regression tests with the current setup (running with a Chrome installed on CircleCI). We had stable visual diff between two runs of the continuous integration, it was enough to only leverage the isolated environment CircleCI provides.
I don't think that we should introduce Docker just yet. Will it only complexify the CI with no expected return? At least, I think that we should wait to see how that goes in practice.

My previous paragraph advocating against using Docker assumes that running the visual regression tests locally is interesting in less than 1% of the pull requests, my experience so far. A workflow that we have battle-tested and has been working great is:

  • We ignore the visual regression tests when working on a pull request.
  • We push our branch.
  • The CI (Argos-CI) compares the generated screenshot during the build with the latest one saved for the baseline branch (master).
  • If something is reported, we open the problematic case in the browser, we notice the regression.
  • We fix the regression, we commit, and push, we wait for the CI to run again.
  • If a small change is still reported by Argos-CI, and is acceptable, we manually approve it (in Argos-CI UI) and we merge the pull request.
  • The merged changes automatically become the baseline for the other pull requests to be compared against.

@oliviertassinari oliviertassinari self-assigned this Aug 16, 2020
@oliviertassinari oliviertassinari mentioned this pull request Aug 27, 2020
13 tasks
@oliviertassinari oliviertassinari marked this pull request as draft December 21, 2020 13:28
@oliviertassinari oliviertassinari removed their assignment Dec 23, 2020
@dtassone
Copy link
Member Author

Closing this one as we have not worked on it since August and we need to reevaluate our approach on how to integrate the tests in the CI.

@dtassone dtassone closed this Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants