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

Adicionar Cypress #8

Merged
merged 7 commits into from
Apr 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ version: 2
jobs:
build:
docker:
- image: circleci/node
- image: cypress/base:8

working_directory: ~/repo

Expand All @@ -21,5 +21,4 @@ jobs:
- node_modules
key: v1-dependencies-{{ checksum "package.json" }}


- run: npm test
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
node_modules/
*.swp
public/dist.js
public/dist.js
cypress/videos
12 changes: 12 additions & 0 deletions bin/cypress-run.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const cypress = require('cypress')
const serverManager = require('./test-server-manager')

Choose a reason for hiding this comment

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

Qual versão do node estamos utilizando? Acho que seria interessante utilizar a última versão + plugins do babel para que possamos utilizar a syntax full ES6. Ficaria muito mais idiomático e fácil de entender isso aqui: import cypress from 'cypress'.

O que pensam disso?

Copy link
Member Author

Choose a reason for hiding this comment

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

Acho que o overhead (tempo de transpilação, muitos pacotes adicionais e configurações) adicionado pelo Babel não compensa o seu uso.

Sobre a questão da versão do node, é uma ótima pergunta! 😃 .

Idealmente, deveríamos configurar o npm e nossas demais dependências (Circle e Heroku, por exemplo) para reforçar o uso de uma versão recente e específica do node (talvez a 8 ou a 9).

Creio que seria uma boa criar uma issue para isso.

Acredito que estas versões mais recentes do node suportem praticamente todas as features do ES6, com exceção de import/export e async/await, se não me engano.

Eu acredito que usar require em vez de import seja menos doloroso do que configurar o Babel no momento.

Faz sentido?


serverManager
.start()
.then((runningServer) => {
cypress
.run()
.then(runningServer.stop)
})
.catch(error => console.log(error.message))

20 changes: 20 additions & 0 deletions bin/test-server-manager.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
const server = require('../server')

Choose a reason for hiding this comment

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

Existe uma variável de ambiente chamada NODE_PATH que serve para que o node saiba onde buscar suas libs.
Por default essa variável tem NODE_PATH=./node_modules/ e por isso que se pode importar as libs assim:
import cypress from 'cypress' (es6). Se nós adicionarmos as pastar ./src e ./test a variável NODE_PATH
poderemos fazer improting de nossos próprios módulos desta forma também. Isso ajuda a deixar o código
mais claro porque evita aquele import wea from '../../../../../wea'.

O que acham disso?

Copy link
Member Author

Choose a reason for hiding this comment

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

Faz sentido. Eu estava receoso de que o NODE_PATH se tornaria obsoleto, mas parece que o pessoal voltou atrás:

nodejs/node#1627

Outro ponto é o de que estamos usando Jest, que possui seu próprio mecanismo de configuração do path, talvez faça sentido ver isso também.

Copy link
Member Author

Choose a reason for hiding this comment

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

Criei uma issue para trabalharmos nisso: #13

const serverStopper = (runningServer) => ({
stop: () => {
runningServer.close()
console.log('Test server stopped')
}
})

Choose a reason for hiding this comment

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

Acho que essa closure ficou um pouco complexa. Talvez seja mais simples só fazer:

const stopServer = (server) => {
  server.close();
  console.log('Test server stopped');
  return server;
}

Mais duas coisas:

  1. Acho que é uma boa prática sempre retornar algo
    2 "server" me pareceu um nome um pouco melhor do que "runningServer". Se a função se chama stop ela deveria saber lidar com um servidor rodando ou não. Em teoria tu poderia perguntar para o servidor se ele está rodando e se não estiver fazer o .close().

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Faz todo sentido, criei uma nova PR para simplifcar essa treta ☝️

Copy link
Member Author

Choose a reason for hiding this comment

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

Acho que é uma boa prática sempre retornar algo

Eu desconheço essa prática, pelo menos em JavaScript. Entendo que esta é uma prática muito relevante em Java, onde retornar um null em lugares inesperados cause NullPointerExceptions aleatórios.

Acho que é seguro não retornar "nada" (funções retornam undefined quando não há return), pelo menos nessa situação aqui.

Choose a reason for hiding this comment

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

Não é um problema grave não retornar nada, mas existem algumas razões do porque desta prática ser adotada:

  1. Funções que retornam algo podem ser mais fáceis de testar. Pois contém entrada e saída.
  2. Retornar algo permite chainability. Retornar o server te permite fazer algo assim por exemplo: stopServer(server).start().stop().start().restart().etc()

E quanto a questão do undefined. Undefined não é útil no restante do codebase e pode levar a erros tipo could not call function on undefined. Também não permite composição que é uma feature interessante de se utilizar (Builder Pattern, por exemplo). Como tu compõe uma função que não retorna um valor útil?

Não é nada muito grave, mas aqui tem alguns exemplos do porque que é uma boa prática sempre retornar algo :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Não havia pensado por esse lado, faz todo sentido!

Vejo muito valor nos pontos que tu citou, inclusive, uma função que não retorna nada geralmente indica sinais de side effects (que é o que acontece nessa situação aqui, por exemplo). Acho que talvez essa seja uma situação mais específica, já que estamos fazendo um "wrapping" do express, que não é lá muito funcional hehehe.

Valeu pela explicação! 😃

const serverStarter = {

Choose a reason for hiding this comment

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

Mesmo comentário que ali em cima

Copy link
Member Author

Choose a reason for hiding this comment

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

start: () => new Promise((resolve, reject) => {
const runningServer = server().listen(3000, () => {
console.log('Test server started at ', 3000)

return resolve(serverStopper(runningServer))
})
})
}

module.exports = serverStarter
4 changes: 4 additions & 0 deletions cypress.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"videoRecording": false,
"baseUrl": "http://localhost:3000"
}
13 changes: 13 additions & 0 deletions cypress/integration/project-list.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
describe('Project List', () => {
it('loads successfully', () => {
cy.visit('/')
})

it('has a title', () => {
cy.get('h1').should('contain', 'Projetos')
})

it('has the hardcoded project', () => {
cy.get('ul > li:first').should('contain', 'Projeto 01')
})
})
17 changes: 17 additions & 0 deletions cypress/plugins/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// ***********************************************************
// This example plugins/index.js can be used to load plugins
//
// You can change the location of this file or turn off loading
// the plugins file with the 'pluginsFile' configuration option.
//
// You can read more here:
// https://on.cypress.io/plugins-guide
// ***********************************************************

// This function is called when a project is opened or re-opened (e.g. due to
// the project's config changing)

module.exports = (on, config) => {
// `on` is used to hook into various events Cypress emits
// `config` is the resolved Cypress config
}
25 changes: 25 additions & 0 deletions cypress/support/commands.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// ***********************************************
// This example commands.js shows you how to
// create various custom commands and overwrite
// existing commands.
//
// For more comprehensive examples of custom
// commands please read more here:
// https://on.cypress.io/custom-commands
// ***********************************************
//
//
// -- This is a parent command --
// Cypress.Commands.add("login", (email, password) => { ... })
//
//
// -- This is a child command --
// Cypress.Commands.add("drag", { prevSubject: 'element'}, (subject, options) => { ... })
//
//
// -- This is a dual command --
// Cypress.Commands.add("dismiss", { prevSubject: 'optional'}, (subject, options) => { ... })
//
//
// -- This is will overwrite an existing command --
// Cypress.Commands.overwrite("visit", (originalFn, url, options) => { ... })
20 changes: 20 additions & 0 deletions cypress/support/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// ***********************************************************
// This example support/index.js is processed and
// loaded automatically before your test files.
//
// This is a great place to put global configuration and
// behavior that modifies Cypress.
//
// You can change the location of this file or turn off
// automatically serving support files with the
// 'supportFile' configuration option.
//
// You can read more here:
// https://on.cypress.io/configuration
// ***********************************************************

// Import commands.js using ES2015 syntax:
import './commands'

// Alternatively you can use CommonJS syntax:
// require('./commands')
6 changes: 3 additions & 3 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
const express = require('express')
const path = require('path')
const server = require('./server')

const PORT = process.env.PORT || 3000

express()
.use(express.static(path.join(__dirname, 'public')))
.listen(PORT, () => console.log('Up on port', PORT))
server()
.listen(PORT, () => console.log('Up on port', PORT))
Loading