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

O que é um PR válido? #2

Open
rodrigondec opened this issue Apr 25, 2020 · 15 comments
Open

O que é um PR válido? #2

rodrigondec opened this issue Apr 25, 2020 · 15 comments
Assignees
Labels
Prioridade: Crítico Isso deve ser resolvido IMEDIATAMENTE. Não ajeitar isso pode ocasionar problemas serios Status: Em Progresso Trabalho em progresso Status: Precisa De Mais Informações Está faltando informações cruciais para prosseguir Tipo: Discussão Discussão sobre mudanças ou decisões Tipo: Documentação Modificação apenas na documentação

Comments

@rodrigondec
Copy link
Member

rodrigondec commented Apr 25, 2020

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 rodrigondec added Prioridade: Crítico Isso deve ser resolvido IMEDIATAMENTE. Não ajeitar isso pode ocasionar problemas serios Status: Bloqueado Isso está bloqueado por algo Status: Precisa De Mais Informações Está faltando informações cruciais para prosseguir Tipo: Documentação Modificação apenas na documentação Tipo: Discussão Discussão sobre mudanças ou decisões labels Apr 25, 2020
@rodrigondec rodrigondec changed the title o que é um PR válido? O que é um PR válido? Apr 25, 2020
@pabrrs
Copy link
Member

pabrrs commented Apr 27, 2020

@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

@ftathiago
Copy link

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.

@rodrigondec
Copy link
Member Author

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

@fdiasr
Copy link

fdiasr commented Apr 28, 2020

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.

@rodrigondec
Copy link
Member Author

rodrigondec commented Apr 28, 2020

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

@fdiasr
Copy link

fdiasr commented Apr 29, 2020

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

@pabrrs
Copy link
Member

pabrrs commented Apr 30, 2020

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 👏

@pabrrs
Copy link
Member

pabrrs commented Apr 30, 2020

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](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 ?

@fdiasr
Copy link

fdiasr commented May 1, 2020

Pessoal, contem comigo para formular isso e utilizar uma ferramenta para validarmos o comportamento após o desenvolvimento.

@rodrigondec
Copy link
Member Author

@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).

@fdiasr
Copy link

fdiasr commented May 4, 2020

@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
Copy link
Member Author

@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

@fdiasr
Copy link

fdiasr commented May 5, 2020

@rodrigondec entrei no discord, fiz um comentario no backend chanell

@pictos pictos added Status: Em Progresso Trabalho em progresso and removed Status: Bloqueado Isso está bloqueado por algo labels May 5, 2020
@ftathiago
Copy link

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).

@pictos pictos closed this as completed May 30, 2020
@rodrigondec
Copy link
Member Author

Reaberto por não ter definições sobre isso no Contributing.md

@rodrigondec rodrigondec reopened this Jun 3, 2020
rodrigondec pushed a commit that referenced this issue Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Prioridade: Crítico Isso deve ser resolvido IMEDIATAMENTE. Não ajeitar isso pode ocasionar problemas serios Status: Em Progresso Trabalho em progresso Status: Precisa De Mais Informações Está faltando informações cruciais para prosseguir Tipo: Discussão Discussão sobre mudanças ou decisões Tipo: Documentação Modificação apenas na documentação
Projects
None yet
Development

No branches or pull requests

5 participants