O que é um PR válido?
rodrigondec opened this issue · 15 comments
Resumo
Após termos o #1 concluídos, precisamos decidir e documentar quais os requisitos que utilizaremos para julgar o que é um PR válido.
Já temos alguns pontos. Mas precisamos expandir.
Explicar nossos 'padrões' de projeto e qualidade de código para ser um PR aceitável (isso pode ser adicionado posteriormente pois ainda será descutido com os tech leads).
Explicar também o que deve acontecer se for um PR inválido.
Exemplos
Por exemplo:
- explicar que para ter um PR é nesserário tem uma Issue aberta
- explicar que teremos testes de CI (obrigatórios) que se não passarem seu PR não será revisado (explicar quais testes)
- explicar os padrões adotados
- linting
- linguagem
- docstring
- documentação
- adição de testes se for uma feature ou mudança de comportamento
Esse issue existe em outros repos
Temos esse issue com a mesma discussão no idvogados.github.io e frontend. Utilizar esse issue para discutir.
idvogados/idvogados.github.io#2
idvogadosorg/idvogados-web#2
@rodrigondec concordo com os pontos que você levantou, acho que é esse o caminho.
Tenho algumas sugestões dentre os tópicos:
Automatizações
explicar que teremos testes de CI (obrigatórios) que se não passarem seu PR não será revisado (explicar quais testes)
Acredito que essa verificação será explicita nos checks do github com o integrador de CI, sendo a validação obrigatória para a versão do Node que utilizaremos, exemplo:
- Node 12 (Obrigatoria)
- Node 14 (Opcional)
Em cada etapa dessa, devem ser rodados todos os testes do projeto.
Além desses steps, acho que podemos ter as verificações de estilo de código, para garantir que o novo código que está sendo sugerido, segue o padrão definido pelo nosso linter. Podemos utilizar um script que roda uma verificação e caso de erro, esse step quebre no CI e impeça que o código seja mergeado, forçando o dev a corrigir os erros de estilo. É possível fazer isso com um comando semelhante a:
"eslint:check": "./node_modules/.bin/eslint .",
"prettier:check": "./node_modules/.bin/prettier --check \"**/{*.js,*.json,bin/**}\"",
"style:check": "npm run prettier:check && npm run eslint:check",Dessa forma, o comando npm run style:check valida o estilo de código definidos pelas regras do prettier e eslint, caso ele encontre alguma irregularidade, o comando retorna um erro.
Como sugestão de regras para estilo, deixo aqui um projeto de boilerplate que utilizo na empresa onde trabalho, que já possui regras definidas para o linter com base no guia de estilos do AirBnB.
Sugestão de regras do ESlint
Sugestão de regras do Prettier
Outro ponto que gostaria de levantar é a cobertura de código. Acho que é bem importante nos checks de uma PR ter a verificação da cobertura do branch, onde esta deve ser maior ou igual a cobertura do branch para onde a PR está sendo mergeada.
Assim, teremos uma maior garantia que todo novo código ou código alterado, terá ao menos um teste para ele. Podemos usar o CodeCov como plataforma para integrar.
O que devemos revisar em uma PR
Imagino que o trabalho de revisão "manual" é tudo aquilo que não conseguimos automatizar. Portanto, penso que uma revisão de PR deve cobrir os segunites tópicos:
- O código novo/alterado está passando nos steps obrigatórios ?
- Todos os testes estão passando
- O estilo de código está de acordo com a regra
- A cobertura do código é maior ou igual a cobertura do branch de destino
- O código novo/alterado resolve a sua Issue ?
- O código novo/alterado possui documentação (se necessário)?
- O código novo/alterado mantem o padrão arquitetural do projeto (MVP, MVVM, MVC) ?
O que acham ?
Perdão pelo textão
Referências
Outra etapa possivel de adicionar antes da review é o sonarqube. Se quebrar as regras do sonar, não passa por review. A não ser que isso seja acordado com algum revisor.
Se não me engano, o sonar é free para open source. E é possível, também, colocar ele no docker para que a pessoa dev possa rodá-lo antes de abrir o PR.
Outra etapa possivel de adicionar antes da review é o sonarqube. Se quebrar as regras do sonar, não passa por review. A não ser que isso seja acordado com algum revisor.
Se não me engano, o sonar é free para open source. E é possível, também, colocar ele no docker para que a pessoa dev possa rodá-lo antes de abrir o PR.
Nunca utilizei ele. Sempre utilizo o Codacy + snyk. É algo muito bom de se ter sim
Em muitos projetos que trabalho utilizamos um card/issue para definir o entregável.
Nesse card definimos criterios de aceite baseados no Requisito de Negócio.
Assim podemos definir testes de comportamento (BDD), para garantir a entrega de valor.
Em muitos projetos que trabalho utilizamos um card/issue para definir o entregável.
Nesse card definimos criterios de aceite baseados no Requisito de Negócio.
Assim podemos definir testes de comportamento (BDD), para garantir a entrega de valor.
@fdiasr coloca uma referência disso para podermos ver.
Estamos almejando que nossos issues tenham os requisitos funcionais de aceite, mas não utilizei BDD e como poderíamos documentar isso como critério de aceite
BDD é um processo de desenvolvimento do produto que possibilita o uso de linguagem ubiqua, utilizando contexto de negócio e minizando o uso da linguagem e detalhes técnicos.
Podemos definir um uma entrega de funcionalidade sendo descritivo quanto a mesma da seguinte forma:
Funcionalidade: Carrinho de produtos
A fim de comprar produtos
Como um cliente
Eu preciso colocar produtos do meu interesse no carrinho
Regras:
- O frete para um carrinho de compras até R$10 é R$4
- O frete para um carrinho de compras maior que R$10 é R$3
Existe um padrão de Contexto (Given) -> Ação (When) -> Resultado (Then) na definição dos cenários que ajuda na estruturação da escrita dos cenarios:
Cenário: Comprando um único produto que custe menos que R$10
**Dado** que exista um "Sabre de luz do Lorde Sith", que custe R$5
**Quando** Eu adicionar o "Sabre de luz do Lorde Sith" ao carrinho
**Então** Eu devo ter 1 produto no carrinho
E o valor total do carrinho deve ser de R$9
Essa descrição fica objetiva para quem faz gestão do projeto e ao mesmo temo legivel para qualquer desevolvedor, pessoa de qualidade ou pessoa de ux/design, executar suas tarefas afim de atender a demanda.
Como bonus existem ferramentas em grade partes das linguagens que executam suites de testes baseadas nesse formato. A criação dos cenários é feita antes do desenvolvimento e os mesmos são utilizados para validação após o desenvolvimento.
- exemplos extraidos da documentação do Behat
Outra etapa possivel de adicionar antes da review é o sonarqube. Se quebrar as regras do sonar, não passa por review. A não ser que isso seja acordado com algum revisor.
Se não me engano, o sonar é free para open source. E é possível, também, colocar ele no docker para que a pessoa dev possa rodá-lo antes de abrir o PR.
@ftathiago ótima recomendação, apoio
BDD é um processo de desenvolvimento do produto que possibilita o uso de linguagem ubiqua, utilizando contexto de negócio e minizando o uso da linguagem e detalhes técnicos.
Podemos definir um uma entrega de funcionalidade sendo descritivo quanto a mesma da seguinte forma:
Funcionalidade: Carrinho de produtos A fim de comprar produtos Como um cliente Eu preciso colocar produtos do meu interesse no carrinho Regras: - O frete para um carrinho de compras até R$10 é R$4 - O frete para um carrinho de compras maior que R$10 é R$3Existe um padrão de Contexto (Given) -> Ação (When) -> Resultado (Then) na definição dos cenários que ajuda na estruturação da escrita dos cenarios:
Cenário: Comprando um único produto que custe menos que R$10 **Dado** que exista um "Sabre de luz do Lorde Sith", que custe R$5 **Quando** Eu adicionar o "Sabre de luz do Lorde Sith" ao carrinho **Então** Eu devo ter 1 produto no carrinho E o valor total do carrinho deve ser de R$9Essa descrição fica objetiva para quem faz gestão do projeto e ao mesmo temo legivel para qualquer desevolvedor, pessoa de qualidade ou pessoa de ux/design, executar suas tarefas afim de atender a demanda.
Como bonus existem ferramentas em grade partes das linguagens que executam suites de testes baseadas nesse formato. A criação dos cenários é feita antes do desenvolvimento e os mesmos são utilizados para validação após o desenvolvimento.
* exemplos extraidos da documentação do [Behat](https://docbehat.readthedocs.io/pt/v3.1/quick_intro_pt1.html)
@fdiasr muito bacana esse modelo de trabalho, nunca tinha visto dessa forma. Apoio em utilizarmos como template de Issue.
O que acham @idvogados/backend ?
Pessoal, contem comigo para formular isso e utilizar uma ferramenta para validarmos o comportamento após o desenvolvimento.
@fdiasr abrimos um formulário para tech leads a um tempo atrás. Você respondeu?
Caso tenha interesse, estamos precisando de pessoas para nos ajudar a organizar fluxos (tal como esse que você sugeriu se voluntariou a fazer).
@rodrigondec nao respondi, entrei recentemente no grupo do slack, e consequentemente acesso ao github do projeto. Podemos ver sim se consigo atuar de forma que ajude mais o time a desenvolver e se organizar
@rodrigondec nao respondi, entrei recentemente no grupo do slack, e consequentemente acesso ao github do projeto. Podemos ver sim se consigo atuar de forma que ajude mais o time a desenvolver e se organizar
Você está no discord (se não estiver peço que entre)? Me manda seu usuário
@rodrigondec entrei no discord, fiz um comentario no backend chanell
Codacy + snyk
Outra etapa possivel de adicionar antes da review é o sonarqube. Se quebrar as regras do sonar, não passa por review. A não ser que isso seja acordado com algum revisor.
Se não me engano, o sonar é free para open source. E é possível, também, colocar ele no docker para que a pessoa dev possa rodá-lo antes de abrir o PR.Nunca utilizei ele. Sempre utilizo o Codacy + snyk. É algo muito bom de se ter sim
@rodrigondec , eu não trabalhei ainda com o Codacy e nem com o snyk. Olhando aqui no site do Codacy ele é pago até para uso local - o que não seria ideal. O sonarqube tem conectores com o mocha e valida as regras à partir do lint (ele tbm tem um pacote de regras próprio que poderia ser utilizado). Para projetos opensource ele disponibiliza gratuitamente uma cloud (sonarcloud), o que poderia até diminuir o tráfego da nossa nuvem.
Se a galera quiser validar local, tbm tem um docker bem sussa de instalar e usar (tudo legal).
Reaberto por não ter definições sobre isso no Contributing.md