-
Notifications
You must be signed in to change notification settings - Fork 1
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
Conversation
@@ -0,0 +1,12 @@ | |||
const cypress = require('cypress') | |||
const serverManager = require('./test-server-manager') |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
@@ -0,0 +1,20 @@ | |||
const server = require('../server') | |||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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
console.log('Test server stopped') | ||
} | ||
}) | ||
|
There was a problem hiding this comment.
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:
- 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().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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 ☝️
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Funções que retornam algo podem ser mais fáceis de testar. Pois contém entrada e saída.
- 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 :)
There was a problem hiding this comment.
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 = { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,6 @@ | |||
const express = require('express') | |||
const {join} = require('path') | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acho que seria uma boa utilizarmos algum linter com algum preset como airbnb ou google. Vejo que muitos destes styleguides recomendam dar espaços entre os {}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
De acordo. Vou criar uma issue para trabalharmos no linter.
@@ -9,9 +9,11 @@ | |||
"docker:db:console": "docker exec -it votacao-db mongo", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eu sou um pouco contra utilizar nodescripts. Tenho fortes argumentos para utilizar Makefiles. Posso depois criar um PR para explicar o porque disso.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eu não curto Makefiles hehehehe. Acredito que a sintaxe deles seja muito confusa às vezes. Seria muito massa trocarmos uma ideia sobre isso! Assim que tu abrir a PR conversamos mais. 😃
Concordo contigo sobre a utilização dos npm scripts. Depois de um ponto eles se tornam caóticos (já estão, na verdade). Concordo que deveríamos buscar um mecanismo melhor.
Caso a gente não chegue em um consenso, podemos usar Jakefiles 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makefile Pros vs NPM Scripts
- Make targets são mais maleáveis. Eu posso copiar eles e colar em qualquer terminal, no CI, etc.
NPM scripts, por outro lado, não rodam no terminal sem passar por algumas regras especiais do NPM. - Conhecimento de stack mais simples. Não seria necessário mais saber como Make e as regras do NPM devem
trabalhar juntas. Só Make, que é uma ferramente unix estável. - Makefiles possuem um mecanismo poderoso de declaração de variável, o que ajuda a remover duplicação dos scripts.
(exemplo: mocha options e variáveis de ambiente de testes). - Makefiles possuem comentários e documentação. package.json, sendo um arquivo json, não possui.
- Makefiles possuem multi-line commands, isso melhora a readability dos nossos scripts.
- Makefile tem autocomplete. Tu pressiona TAB e aparece todos os targets existentes no projeto.
- Makefiles podem ser quebrados em sub makefiles usando a diretiva include (melhor organização).
Enquanto no package.json tu coloca tudo misutrado em um arquivo só - Makefiles possuem dependencias de targets, podendo fazer várias coisas com um comando só.
Exemplo: start: clean build
Makefile Cons vs NPM Scripts
- Makefiles são mais verbosos para lidar com os binários do node_modules. Porem pode-se remover a duplicação
usando a variável NPM_BIN. - Makefile é uma ferramenta unix muito complexa e pode demorar muito para tornar-se proeficiente.
Pode tornar-se bem complexa.
Essas são algumas coisas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makefiles são mais verbosos para lidar com os binários do node_modules. Porem pode-se remover a duplicação usando a variável NPM_BIN.
Para isso, podemos usar o npx, fica ainda mais simples! No npm >= 5, ele vem instalado por padrão, não precisamos instalá-lo separadamente.
Makefile é uma ferramenta unix muito complexa e pode demorar muito para tornar-se proeficiente. Pode tornar-se bem complexa.
Essa é a minha maior queixa à respeito dos Makefiles.
Às vezes eles se tornam complexos demais para resolver problemas muito pequenos.
Acredito que isso se deva ao fato de usarmos o make para resolver um problema que ele não foi feito para resolver, usamos ele para gerenciar alias de scripts, quando na verdade ele serve para outras coisas:
GNU Make is a tool which controls the generation of executables and other non-source files of a program from the program's source files.
Make gets its knowledge of how to build your program from a file called the makefile, which lists each of the non-source files and how to compute it from other files. When you write a program, you should write a makefile for it, so that it is possible to use Make to build and install the program.
Eu acredito que a gente possa adotar alguma ferramenta mais simples, ou mais direcionada ao nosso propósito. Talvez possamos pensar em ferramentas como Grunt, por exemplo. Ou mais simples ainda, uma pasta /bin
com scripts JavaScript e/ou shell.
Faz sentido?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Faz sim. Dado que ainda não temos nada muito complexo. Mas são discussões úteis de se levantar no projeto da turma :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Com certeza! Muito obrigado por levantar o ponto dos makefiles! :)
Configuração inicial do Cypress.
Adicionado/alterado: